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

[PATCH v7 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 v5:
 - Remove the notifier chain things from the gadget and introduce one callback
 function to report to the usb charger when the gadget state is changed.
 - Flesh out the port type detection which combines the USB negotiation and
 PMICs detection.
 - Supply the notification mechanism to userspace when charger state is changed.
 - Integrate with the vbus staff in the gadget API.
 - Spilt up the functionality for userspace with one file per USB charger type.
 - Rebase on "4.5-rc4".

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

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

-- 
1.7.9.5

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

[Baolin Wang]

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

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

The usb charger will register notifiees on the usb gadget or the extcon
device to get notified the usb charger state. It also supplies the
notification mechanism to userspace When the usb charger state is changed.

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

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

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/gadget/Kconfig      |    7 +
 drivers/usb/gadget/Makefile     |    1 +
 drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usb_charger.h |  164 ++++++++++
 4 files changed, 841 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index af5d922..82a5b3c 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
 	help
 	   It supports the serial gadget can be used as a console.
 
+config USB_CHARGER
+	bool "USB charger support"
+	help
+	  The usb charger driver based on the usb gadget that makes an
+	  enhancement to a power driver which can set the current limitation
+	  when the usb charger is added or removed.
+
 source "drivers/usb/gadget/udc/Kconfig"
 
 #
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 598a67d..1e421c1 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
 libcomposite-y			+= composite.o functions.o configfs.o u_f.o
 
 obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
+obj-$(CONFIG_USB_CHARGER)	+= charger.o
diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
new file mode 100644
index 0000000..82a9973
--- /dev/null
+++ b/drivers/usb/gadget/charger.c
@@ -0,0 +1,669 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/extcon.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/usb_charger.h>
+#include <linux/power_supply.h>
+
+#define DEFAULT_CUR_PROTECT	(50)
+#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define UCHGER_STATE_LENGTH	(50)
+
+static DEFINE_IDA(usb_charger_ida);
+static struct bus_type usb_charger_subsys = {
+	.name           = "usb-charger",
+	.dev_name       = "usb-charger",
+};
+
+static struct usb_charger *dev_to_uchger(struct device *udev)
+{
+	return container_of(udev, struct usb_charger, dev);
+}
+
+static ssize_t sdp_limit_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
+}
+
+static ssize_t sdp_limit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	unsigned int sdp_limit;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &sdp_limit);
+	if (ret < 0)
+		return ret;
+
+	ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(sdp_limit);
+
+static ssize_t dcp_limit_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%d\n", uchger->cur_limit.dcp_cur_limit);
+}
+
+static ssize_t dcp_limit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	unsigned int dcp_limit;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &dcp_limit);
+	if (ret < 0)
+		return ret;
+
+	ret = usb_charger_set_cur_limit_by_type(uchger, DCP_TYPE, dcp_limit);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(dcp_limit);
+
+static ssize_t cdp_limit_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%d\n", uchger->cur_limit.cdp_cur_limit);
+}
+
+static ssize_t cdp_limit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	unsigned int cdp_limit;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &cdp_limit);
+	if (ret < 0)
+		return ret;
+
+	ret = usb_charger_set_cur_limit_by_type(uchger, CDP_TYPE, cdp_limit);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(cdp_limit);
+
+static ssize_t aca_limit_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%d\n", uchger->cur_limit.aca_cur_limit);
+}
+
+static ssize_t aca_limit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	unsigned int aca_limit;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &aca_limit);
+	if (ret < 0)
+		return ret;
+
+	ret = usb_charger_set_cur_limit_by_type(uchger, ACA_TYPE, aca_limit);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(aca_limit);
+
+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_sdp_limit.attr,
+	&dev_attr_dcp_limit.attr,
+	&dev_attr_cdp_limit.attr,
+	&dev_attr_aca_limit.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(usb_charger);
+
+/*
+ * usb_charger_find_by_name - Get the usb charger device by name.
+ * @name - usb charger device name.
+ *
+ * return the instance of usb charger device, the device must be
+ * released with usb_charger_put().
+ */
+struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	struct device *udev;
+
+	if (!name)
+		return ERR_PTR(-EINVAL);
+
+	udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
+	if (!udev)
+		return ERR_PTR(-ENODEV);
+
+	return dev_to_uchger(udev);
+}
+EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
+
+/*
+ * usb_charger_get() - Reference a usb charger.
+ * @uchger - usb charger
+ */
+struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get);
+
+/*
+ * usb_charger_put() - Dereference a usb charger.
+ * @uchger - charger to release
+ */
+void usb_charger_put(struct usb_charger *uchger)
+{
+	if (uchger)
+		put_device(&uchger->dev);
+}
+EXPORT_SYMBOL_GPL(usb_charger_put);
+
+/*
+ * usb_charger_register_notify() - Register a notifiee to get notified by
+ * any attach status changes from the usb charger detection.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be registered.
+ */
+int usb_charger_register_notify(struct usb_charger *uchger,
+				struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
+
+	/* Generate an initial notify so users start in the right state */
+	if (!ret) {
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					usb_charger_get_cur_limit(uchger),
+					uchger);
+	}
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_charger_register_notify);
+
+/*
+ * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be unregistered.
+ */
+int usb_charger_unregister_notify(struct usb_charger *uchger,
+				  struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		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_detect_type() - Get the usb charger type by the callback
+ * which is implemented by gadget operations.
+ * @uchger - the usb charger device.
+ *
+ * return the usb charger type.
+ */
+enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	if (uchger->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;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);
+
+/*
+ * usb_charger_set_cur_limit_by_type() - Set the current limitation
+ * by charger type.
+ * @uchger - the usb charger device.
+ * @type - the usb charger type.
+ * @cur_limit - the current limitation.
+ */
+int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				      enum usb_charger_type type,
+				      unsigned int cur_limit)
+{
+	if (!uchger)
+		return -EINVAL;
+
+	switch (type) {
+	case SDP_TYPE:
+		uchger->cur_limit.sdp_cur_limit = cur_limit;
+		break;
+	case DCP_TYPE:
+		uchger->cur_limit.dcp_cur_limit = cur_limit;
+		break;
+	case CDP_TYPE:
+		uchger->cur_limit.cdp_cur_limit	= cur_limit;
+		break;
+	case ACA_TYPE:
+		uchger->cur_limit.aca_cur_limit	= cur_limit;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
+
+/*
+ * usb_charger_set_cur_limit() - Set the current limitation.
+ * @uchger - the usb charger device.
+ * @cur_limit_set - the current limitation.
+ */
+int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set)
+{
+	if (!uchger || !cur_limit_set)
+		return -EINVAL;
+
+	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
+	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
+	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
+	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit);
+
+/*
+ * usb_charger_get_cur_limit() - Get the current limitation by
+ * different usb charger type.
+ * @uchger - the usb charger device.
+ *
+ * return the current limitation to set.
+ */
+unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	enum usb_charger_type uchger_type = usb_charger_detect_type(uchger);
+	unsigned int cur_limit;
+
+	switch (uchger_type) {
+	case SDP_TYPE:
+		cur_limit = uchger->cur_limit.sdp_cur_limit;
+		break;
+	case DCP_TYPE:
+		cur_limit = uchger->cur_limit.dcp_cur_limit;
+		break;
+	case CDP_TYPE:
+		cur_limit = uchger->cur_limit.cdp_cur_limit;
+		break;
+	case ACA_TYPE:
+		cur_limit = uchger->cur_limit.aca_cur_limit;
+		break;
+	default:
+		return 0;
+	}
+
+	return cur_limit;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_cur_limit);
+
+/*
+ * usb_charger_notifier_others() - It will notify other device registered
+ * on usb charger when the usb charger state is changed.
+ * @uchger - the usb charger device.
+ * @state - the state of the usb charger.
+ */
+static void
+usb_charger_notify_others(struct usb_charger *uchger,
+			  enum usb_charger_state state)
+{
+	char uchger_state[UCHGER_STATE_LENGTH];
+	char *envp[] = { uchger_state, NULL };
+
+	mutex_lock(&uchger->lock);
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+			usb_charger_get_cur_limit(uchger),
+			uchger);
+		snprintf(uchger_state, UCHGER_STATE_LENGTH,
+			 "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, UCHGER_STATE_LENGTH,
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
+		break;
+	default:
+		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
+			 state);
+		mutex_unlock(&uchger->lock);
+		return;
+	}
+
+	kobject_uevent_env(&uchger->dev.kobj, KOBJ_CHANGE, envp);
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * usb_charger_plug_by_extcon() - The notifier call function which is registered
+ * on the extcon device.
+ * @nb - the notifier block that notified by extcon device.
+ * @state - the extcon device state.
+ * @data - here specify a extcon device.
+ *
+ * return the notify flag.
+ */
+static int
+usb_charger_plug_by_extcon(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_charger_nb *extcon_nb =
+		container_of(nb, struct usb_charger_nb, nb);
+	struct usb_charger *uchger = extcon_nb->uchger;
+	enum usb_charger_state uchger_state;
+
+	if (!uchger)
+		return NOTIFY_BAD;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger is added or removed
+	 * with detecting by extcon device.
+	 */
+	if (state)
+		uchger_state = USB_CHARGER_PRESENT;
+	else
+		uchger_state = USB_CHARGER_REMOVE;
+
+	usb_charger_notify_others(uchger, uchger_state);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * usb_charger_plug_by_gadget() - Set the usb charger current limitation
+ * according to the usb gadget device state.
+ * @gadget - the usb gadget device.
+ * @state - the usb gadget state.
+ */
+int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+			       unsigned long state)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
+
+static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
+{
+	struct usb_charger **r = res;
+
+	if (WARN_ON(!r || !*r))
+		return 0;
+
+	return *r == data;
+}
+
+static void usb_charger_release(struct device *dev)
+{
+	struct usb_charger *uchger = dev_get_drvdata(dev);
+
+	kfree(uchger);
+}
+
+/*
+ * usb_charger_unregister() - Unregister a usb charger device.
+ * @uchger - the usb charger to be unregistered.
+ */
+static int usb_charger_unregister(struct usb_charger *uchger)
+{
+	if (!uchger)
+		return -EINVAL;
+
+	device_unregister(&uchger->dev);
+	return 0;
+}
+
+static void devm_uchger_dev_unreg(struct device *dev, void *res)
+{
+	usb_charger_unregister(*(struct usb_charger **)res);
+}
+
+void devm_usb_charger_unregister(struct device *dev,
+				 struct usb_charger *uchger)
+{
+	devres_release(dev, devm_uchger_dev_unreg,
+		       devm_uchger_dev_match, uchger);
+}
+
+/*
+ * usb_charger_register() - Register a new usb charger device
+ * which is created by the usb charger framework.
+ * @parent - the parent device of the new usb charger.
+ * @uchger - the new usb charger device.
+ */
+static int usb_charger_register(struct device *parent,
+				struct usb_charger *uchger)
+{
+	int ret;
+
+	if (!uchger)
+		return -EINVAL;
+
+	uchger->dev.parent = parent;
+	uchger->dev.release = usb_charger_release;
+	uchger->dev.bus = &usb_charger_subsys;
+	uchger->dev.groups = usb_charger_groups;
+
+	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto fail_ida;
+
+	uchger->id = ret;
+	dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
+	dev_set_drvdata(&uchger->dev, uchger);
+
+	ret = device_register(&uchger->dev);
+	if (ret)
+		goto fail_register;
+
+	return 0;
+
+fail_register:
+	put_device(&uchger->dev);
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+	uchger->id = -1;
+fail_ida:
+	dev_err(parent, "Failed to register usb charger: %d\n", ret);
+	return ret;
+}
+
+int devm_usb_charger_register(struct device *dev,
+			      struct usb_charger *uchger)
+{
+	struct usb_charger **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = usb_charger_register(dev, uchger);
+	if (ret) {
+		devres_free(ptr);
+		return ret;
+	}
+
+	*ptr = uchger;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+
+int usb_charger_init(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger;
+	struct extcon_dev *edev;
+	struct power_supply *psy;
+	int ret;
+
+	if (!ugadget)
+		return -EINVAL;
+
+	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->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;
+	uchger->get_charger_type = NULL;
+
+	mutex_init(&uchger->lock);
+	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
+
+	/* register a notifier on a extcon device if it is exsited */
+	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
+	if (!IS_ERR_OR_NULL(edev)) {
+		uchger->extcon_dev = edev;
+		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
+		uchger->extcon_nb.uchger = uchger;
+		extcon_register_notifier(edev, EXTCON_USB,
+					 &uchger->extcon_nb.nb);
+	}
+
+	/* to check if the usb charger is link to a power supply */
+	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
+					       "power-supplies");
+	if (!IS_ERR_OR_NULL(psy))
+		uchger->psy = psy;
+	else
+		uchger->psy = NULL;
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = ugadget->state;
+
+	/* register a new usb charger */
+	ret = usb_charger_register(&ugadget->dev, uchger);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	kfree(uchger);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+static int __init usb_charger_sysfs_init(void)
+{
+	return subsys_system_register(&usb_charger_subsys, NULL);
+}
+core_initcall(usb_charger_sysfs_init);
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
+MODULE_DESCRIPTION("USB charger driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
new file mode 100644
index 0000000..eed422f
--- /dev/null
+++ b/include/linux/usb/usb_charger.h
@@ -0,0 +1,164 @@
+#ifndef __LINUX_USB_CHARGER_H__
+#define __LINUX_USB_CHARGER_H__
+
+#include <uapi/linux/usb/ch9.h>
+
+/* USB charger type:
+ * SDP (Standard Downstream Port)
+ * DCP (Dedicated Charging Port)
+ * CDP (Charging Downstream Port)
+ * ACA (Accessory Charger Adapters)
+ */
+enum usb_charger_type {
+	UNKNOWN_TYPE,
+	SDP_TYPE,
+	DCP_TYPE,
+	CDP_TYPE,
+	ACA_TYPE,
+};
+
+/* USB charger state */
+enum usb_charger_state {
+	USB_CHARGER_DEFAULT,
+	USB_CHARGER_PRESENT,
+	USB_CHARGER_REMOVE,
+};
+
+/* Current limitation by charger type */
+struct usb_charger_cur_limit {
+	unsigned int sdp_cur_limit;
+	unsigned int dcp_cur_limit;
+	unsigned int cdp_cur_limit;
+	unsigned int aca_cur_limit;
+};
+
+struct usb_charger_nb {
+	struct notifier_block	nb;
+	struct usb_charger	*uchger;
+};
+
+struct usb_charger {
+	struct device		dev;
+	struct raw_notifier_head	uchger_nh;
+	/* protect the notifier head */
+	struct mutex		lock;
+	int			id;
+	enum usb_charger_type	type;
+	enum usb_charger_state	state;
+
+	/* for supporting extcon usb gpio */
+	struct extcon_dev	*extcon_dev;
+	struct usb_charger_nb	extcon_nb;
+
+	/* for supporting usb gadget */
+	struct usb_gadget	*gadget;
+	enum usb_device_state	old_gadget_state;
+
+	/* for supporting power supply */
+	struct power_supply	*psy;
+
+	/* user can get charger type by implementing this callback */
+	enum usb_charger_type	(*get_charger_type)(struct usb_charger *);
+
+	/* current limitation */
+	struct usb_charger_cur_limit	cur_limit;
+};
+
+#ifdef CONFIG_USB_CHARGER
+extern struct usb_charger *usb_charger_find_by_name(const char *name);
+
+extern struct usb_charger *usb_charger_get(struct usb_charger *uchger);
+extern void usb_charger_put(struct usb_charger *uchger);
+
+extern int usb_charger_register_notify(struct usb_charger *uchger,
+				       struct notifier_block *nb);
+extern int usb_charger_unregister_notify(struct usb_charger *uchger,
+					 struct notifier_block *nb);
+
+extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
+				struct usb_charger_cur_limit *cur_limit_set);
+extern int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+					     enum usb_charger_type type,
+					     unsigned int cur_limit);
+extern unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+extern enum usb_charger_type usb_charger_detect_type(struct usb_charger *uchger);
+extern int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+				      unsigned long state);
+
+extern int usb_charger_init(struct usb_gadget *ugadget);
+extern int usb_charger_exit(struct usb_gadget *ugadget);
+#else
+static inline struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return NULL;
+}
+
+static inline void usb_charger_put(struct usb_charger *uchger)
+{
+}
+
+static inline int
+usb_charger_register_notify(struct usb_charger *uchger,
+			    struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_unregister_notify(struct usb_charger *uchger,
+			      struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit(struct usb_charger *uchger,
+			  struct usb_charger_cur_limit *cur_limit_set)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				  enum usb_charger_type type,
+				  unsigned int cur_limit)
+{
+	return 0;
+}
+
+static inline unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	return UNKNOWN_TYPE;
+}
+
+static inline int
+usb_charger_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__ */
-- 
1.7.9.5

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

[Oliver Neukum]

On Wed, 2016-03-16 at 19:46 +0800, Baolin Wang wrote:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
> 
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.

This patch adds sysfs attributes without adding Documentation for them.
That's bad.

	Regards
		Oliver

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

[Baolin Wang]

On 16 March 2016 at 20:09, Oliver Neukum <oneukum@suse.com> wrote:
> On Wed, 2016-03-16 at 19:46 +0800, Baolin Wang wrote:
>> This patch introduces the usb charger driver based on usb gadget that
>> makes an enhancement to a power driver. It works well in practice but
>> that requires a system with suitable hardware.
>>
>> The basic conception of the usb charger is that, when one usb charger
>> is added or removed by reporting from the usb gadget state change or
>> the extcon device state change, the usb charger will report to power
>> user to set the current limitation.
>
> This patch adds sysfs attributes without adding Documentation for them.
> That's bad.

I'll add Documentation for the sysfs attributes in next version
patchset. Thanks for your comments.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
>
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.
>
> The usb charger will register notifiees on the usb gadget or the extcon
> device to get notified the usb charger state. It also supplies the
> notification mechanism to userspace When the usb charger state is changed.
>
> Power user will register a notifiee on the usb charger to get notified
> by status changes from the usb charger. It will report to power user
> to set the current limitation when detecting the usb charger is added
> or removed from extcon device state or usb gadget state.
>
> This patch doesn't yet integrate with the gadget code, so some functions
> which rely on the 'gadget' are not completed, that will be implemented
> in the following patches.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/Kconfig      |    7 +
>  drivers/usb/gadget/Makefile     |    1 +
>  drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++

It seems to me this should be part of udc-core's functionality. Maybe
move this to drivers/usb/gadget/udc and statically link it to
udc-core.ko ?

> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index af5d922..82a5b3c 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
>  	help
>  	   It supports the serial gadget can be used as a console.
>  
> +config USB_CHARGER
> +	bool "USB charger support"
> +	help
> +	  The usb charger driver based on the usb gadget that makes an
> +	  enhancement to a power driver which can set the current limitation
> +	  when the usb charger is added or removed.

sure you don't depend on anything ?

> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 598a67d..1e421c1 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
>  libcomposite-y			+= composite.o functions.o configfs.o u_f.o
>  
>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> +obj-$(CONFIG_USB_CHARGER)	+= charger.o
> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
> new file mode 100644
> index 0000000..82a9973
> --- /dev/null
> +++ b/drivers/usb/gadget/charger.c
> @@ -0,0 +1,669 @@
> +/*
> + * charger.c -- USB charger driver
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>

not very nice to depend on either of or platform_device here. What about
PCI-based devices ?

> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/usb_charger.h>
> +#include <linux/power_supply.h>
> +
> +#define DEFAULT_CUR_PROTECT	(50)

Where is this coming from ? Also, () are not necessary.

> +#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)

According to the spec we should always be talking about unit loads (1
unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
work for SS capable ports and SS gadgets (we have quite a few of them,
actually). You're missing the opportunity of charging at 900mA.

> +#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define UCHGER_STATE_LENGTH	(50)

what is this UCHGER_STATE_LENGTH ? And also, why don't you spell it out?
Is this weird name coming from a spec ? Which spec ?

> +static DEFINE_IDA(usb_charger_ida);
> +static struct bus_type usb_charger_subsys = {
> +	.name           = "usb-charger",
> +	.dev_name       = "usb-charger",
> +};
> +
> +static struct usb_charger *dev_to_uchger(struct device *udev)

nit-picking but I'd rather call this struct device *dev. Also, I'm not
sure this fits as a bus_type. There's no "usb charger" bus. There's USB
bus and its VBUS/GND lines. Why are you using a bus_type here ?

> +{
> +	return container_of(udev, struct usb_charger, dev);
> +}
> +
> +static ssize_t sdp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
> +}
> +
> +static ssize_t sdp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int sdp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &sdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(sdp_limit);

why RW ? Who's going to use these ? Also, you're not documenting this
new sysfs file.

> +static ssize_t dcp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.dcp_cur_limit);
> +}
> +
> +static ssize_t dcp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int dcp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &dcp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, DCP_TYPE, dcp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(dcp_limit);

likewise, why RW ? Missing doc.

> +static ssize_t cdp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.cdp_cur_limit);
> +}
> +
> +static ssize_t cdp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int cdp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &cdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, CDP_TYPE, cdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(cdp_limit);

why RW? Where's doc ?

> +static ssize_t aca_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.aca_cur_limit);
> +}
> +
> +static ssize_t aca_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int aca_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &aca_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, ACA_TYPE, aca_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(aca_limit);

why RW ? where's doc ?

> +static struct attribute *usb_charger_attrs[] = {

const ?

> +	&dev_attr_sdp_limit.attr,
> +	&dev_attr_dcp_limit.attr,
> +	&dev_attr_cdp_limit.attr,
> +	&dev_attr_aca_limit.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(usb_charger);

static ?

> +struct usb_charger *usb_charger_find_by_name(const char *name)
> +{
> +	struct device *udev;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);

quite frankly, this deserves, at a minimum, a big fat WARN():

	if (WARN(!name, "can't work with NULL name"))
		return .....

> +	udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
> +	if (!udev)
> +		return ERR_PTR(-ENODEV);

still not convinced this deserves to be a bus_type.

> +	return dev_to_uchger(udev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
> +
> +/*
> + * usb_charger_get() - Reference a usb charger.
> + * @uchger - usb charger
> + */
> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
> +{
> +	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get);
> +
> +/*
> + * usb_charger_put() - Dereference a usb charger.
> + * @uchger - charger to release
> + */
> +void usb_charger_put(struct usb_charger *uchger)
> +{
> +	if (uchger)
> +		put_device(&uchger->dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_put);
> +
> +/*
> + * usb_charger_register_notify() - Register a notifiee to get notified by
> + * any attach status changes from the usb charger detection.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be registered.
> + */
> +int usb_charger_register_notify(struct usb_charger *uchger,
> +				struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)
> +		return -EINVAL;
> +
> +	mutex_lock(&uchger->lock);
> +	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
> +
> +	/* Generate an initial notify so users start in the right state */
> +	if (!ret) {
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +					usb_charger_get_cur_limit(uchger),
> +					uchger);
> +	}
> +	mutex_unlock(&uchger->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_register_notify);
> +
> +/*
> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be unregistered.
> + */
> +int usb_charger_unregister_notify(struct usb_charger *uchger,
> +				  struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)

WARN() ?

> +		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_detect_type() - Get the usb charger type by the callback
> + * which is implemented by gadget operations.
> + * @uchger - the usb charger device.
> + *
> + * return the usb charger type.
> + */
> +enum usb_charger_type
> +usb_charger_detect_type(struct usb_charger *uchger)
> +{
> +	if (uchger->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;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> +
> +/*
> + * usb_charger_set_cur_limit_by_type() - Set the current limitation
> + * by charger type.
> + * @uchger - the usb charger device.
> + * @type - the usb charger type.
> + * @cur_limit - the current limitation.
> + */
> +int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
> +				      enum usb_charger_type type,
> +				      unsigned int cur_limit)
> +{
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	switch (type) {
> +	case SDP_TYPE:
> +		uchger->cur_limit.sdp_cur_limit = cur_limit;
> +		break;
> +	case DCP_TYPE:
> +		uchger->cur_limit.dcp_cur_limit = cur_limit;
> +		break;
> +	case CDP_TYPE:
> +		uchger->cur_limit.cdp_cur_limit	= cur_limit;
> +		break;
> +	case ACA_TYPE:
> +		uchger->cur_limit.aca_cur_limit	= cur_limit;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
> +
> +/*
> + * usb_charger_set_cur_limit() - Set the current limitation.
> + * @uchger - the usb charger device.
> + * @cur_limit_set - the current limitation.
> + */
> +int usb_charger_set_cur_limit(struct usb_charger *uchger,
> +			      struct usb_charger_cur_limit *cur_limit_set)
> +{
> +	if (!uchger || !cur_limit_set)
> +		return -EINVAL;
> +
> +	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
> +	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
> +	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
> +	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit);
> +
> +/*
> + * usb_charger_get_cur_limit() - Get the current limitation by
> + * different usb charger type.
> + * @uchger - the usb charger device.
> + *
> + * return the current limitation to set.
> + */
> +unsigned int
> +usb_charger_get_cur_limit(struct usb_charger *uchger)
> +{
> +	enum usb_charger_type uchger_type = usb_charger_detect_type(uchger);
> +	unsigned int cur_limit;
> +
> +	switch (uchger_type) {
> +	case SDP_TYPE:
> +		cur_limit = uchger->cur_limit.sdp_cur_limit;
> +		break;
> +	case DCP_TYPE:
> +		cur_limit = uchger->cur_limit.dcp_cur_limit;
> +		break;
> +	case CDP_TYPE:
> +		cur_limit = uchger->cur_limit.cdp_cur_limit;
> +		break;
> +	case ACA_TYPE:
> +		cur_limit = uchger->cur_limit.aca_cur_limit;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return cur_limit;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_cur_limit);
> +
> +/*
> + * usb_charger_notifier_others() - It will notify other device registered
> + * on usb charger when the usb charger state is changed.
> + * @uchger - the usb charger device.
> + * @state - the state of the usb charger.
> + */
> +static void
> +usb_charger_notify_others(struct usb_charger *uchger,
> +			  enum usb_charger_state state)
> +{
> +	char uchger_state[UCHGER_STATE_LENGTH];
> +	char *envp[] = { uchger_state, NULL };
> +
> +	mutex_lock(&uchger->lock);
> +	uchger->state = state;
> +
> +	switch (state) {
> +	case USB_CHARGER_PRESENT:
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +			usb_charger_get_cur_limit(uchger),
> +			uchger);
> +		snprintf(uchger_state, UCHGER_STATE_LENGTH,
> +			 "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, UCHGER_STATE_LENGTH,
> +			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
> +		break;
> +	default:
> +		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
> +			 state);
> +		mutex_unlock(&uchger->lock);
> +		return;
> +	}
> +
> +	kobject_uevent_env(&uchger->dev.kobj, KOBJ_CHANGE, envp);
> +	mutex_unlock(&uchger->lock);
> +}
> +
> +/*
> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
> + * on the extcon device.
> + * @nb - the notifier block that notified by extcon device.
> + * @state - the extcon device state.
> + * @data - here specify a extcon device.
> + *
> + * return the notify flag.
> + */
> +static int
> +usb_charger_plug_by_extcon(struct notifier_block *nb,
> +			   unsigned long state, void *data)
> +{
> +	struct usb_charger_nb *extcon_nb =
> +		container_of(nb, struct usb_charger_nb, nb);
> +	struct usb_charger *uchger = extcon_nb->uchger;
> +	enum usb_charger_state uchger_state;
> +
> +	if (!uchger)
> +		return NOTIFY_BAD;
> +
> +	/* Report event to power to setting the current limitation
> +	 * for this usb charger when one usb charger is added or removed
> +	 * with detecting by extcon device.
> +	 */
> +	if (state)
> +		uchger_state = USB_CHARGER_PRESENT;
> +	else
> +		uchger_state = USB_CHARGER_REMOVE;
> +
> +	usb_charger_notify_others(uchger, uchger_state);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/*
> + * usb_charger_plug_by_gadget() - Set the usb charger current limitation
> + * according to the usb gadget device state.
> + * @gadget - the usb gadget device.
> + * @state - the usb gadget state.
> + */
> +int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
> +			       unsigned long state)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
> +
> +static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
> +{
> +	struct usb_charger **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +static void usb_charger_release(struct device *dev)
> +{
> +	struct usb_charger *uchger = dev_get_drvdata(dev);
> +
> +	kfree(uchger);
> +}
> +
> +/*
> + * usb_charger_unregister() - Unregister a usb charger device.
> + * @uchger - the usb charger to be unregistered.
> + */
> +static int usb_charger_unregister(struct usb_charger *uchger)
> +{
> +	if (!uchger)

WARN()

> +		return -EINVAL;
> +
> +	device_unregister(&uchger->dev);
> +	return 0;
> +}
> +
> +static void devm_uchger_dev_unreg(struct device *dev, void *res)
> +{
> +	usb_charger_unregister(*(struct usb_charger **)res);
> +}
> +
> +void devm_usb_charger_unregister(struct device *dev,
> +				 struct usb_charger *uchger)
> +{
> +	devres_release(dev, devm_uchger_dev_unreg,
> +		       devm_uchger_dev_match, uchger);
> +}

does this need EXPORT_SYMBOL_GPL() too ?

> +/*
> + * usb_charger_register() - Register a new usb charger device
> + * which is created by the usb charger framework.
> + * @parent - the parent device of the new usb charger.
> + * @uchger - the new usb charger device.
> + */
> +static int usb_charger_register(struct device *parent,
> +				struct usb_charger *uchger)
> +{
> +	int ret;
> +
> +	if (!uchger)

WARN()

> +		return -EINVAL;
> +
> +	uchger->dev.parent = parent;
> +	uchger->dev.release = usb_charger_release;
> +	uchger->dev.bus = &usb_charger_subsys;
> +	uchger->dev.groups = usb_charger_groups;
> +
> +	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto fail_ida;
> +
> +	uchger->id = ret;
> +	dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
> +	dev_set_drvdata(&uchger->dev, uchger);
> +
> +	ret = device_register(&uchger->dev);
> +	if (ret)
> +		goto fail_register;
> +
> +	return 0;
> +
> +fail_register:
> +	put_device(&uchger->dev);
> +	ida_simple_remove(&usb_charger_ida, uchger->id);
> +	uchger->id = -1;
> +fail_ida:
> +	dev_err(parent, "Failed to register usb charger: %d\n", ret);
> +	return ret;
> +}
> +
> +int devm_usb_charger_register(struct device *dev,
> +			      struct usb_charger *uchger)
> +{
> +	struct usb_charger **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = usb_charger_register(dev, uchger);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	*ptr = uchger;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +
> +int usb_charger_init(struct usb_gadget *ugadget)
> +{
> +	struct usb_charger *uchger;
> +	struct extcon_dev *edev;
> +	struct power_supply *psy;
> +	int ret;
> +
> +	if (!ugadget)

WARN()

but I don't like this. This should be done from udc-core.c itself when
the UDC registers itself.

> +		return -EINVAL;
> +
> +	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->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;
> +	uchger->get_charger_type = NULL;
> +
> +	mutex_init(&uchger->lock);
> +	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
> +
> +	/* register a notifier on a extcon device if it is exsited */
> +	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
> +	if (!IS_ERR_OR_NULL(edev)) {
> +		uchger->extcon_dev = edev;
> +		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
> +		uchger->extcon_nb.uchger = uchger;
> +		extcon_register_notifier(edev, EXTCON_USB,
> +					 &uchger->extcon_nb.nb);
> +	}
> +
> +	/* to check if the usb charger is link to a power supply */
> +	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
> +					       "power-supplies");
> +	if (!IS_ERR_OR_NULL(psy))
> +		uchger->psy = psy;
> +	else
> +		uchger->psy = NULL;
> +
> +	/* register a notifier on a usb gadget device */
> +	uchger->gadget = ugadget;
> +	uchger->old_gadget_state = ugadget->state;
> +
> +	/* register a new usb charger */
> +	ret = usb_charger_register(&ugadget->dev, uchger);
> +	if (ret)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	if (uchger->extcon_dev)
> +		extcon_unregister_notifier(uchger->extcon_dev,
> +					   EXTCON_USB, &uchger->extcon_nb.nb);
> +
> +	kfree(uchger);
> +	return ret;
> +}
> +
> +int usb_charger_exit(struct usb_gadget *ugadget)
> +{
> +	return 0;
> +}
> +
> +static int __init usb_charger_sysfs_init(void)
> +{
> +	return subsys_system_register(&usb_charger_subsys, NULL);
> +}
> +core_initcall(usb_charger_sysfs_init);

please don't. This should be indication that there's something wrong
with your patchset.

> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
> +MODULE_DESCRIPTION("USB charger driver");
> +MODULE_LICENSE("GPL");

GPLv2 or GPLv2+ ??

> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
> new file mode 100644
> index 0000000..eed422f
> --- /dev/null
> +++ b/include/linux/usb/usb_charger.h

usb_ is redundant. I'd prefer to:

#include <linux/usb/charger.h>

> @@ -0,0 +1,164 @@
> +#ifndef __LINUX_USB_CHARGER_H__
> +#define __LINUX_USB_CHARGER_H__
> +
> +#include <uapi/linux/usb/ch9.h>
> +
> +/* USB charger type:
> + * SDP (Standard Downstream Port)
> + * DCP (Dedicated Charging Port)
> + * CDP (Charging Downstream Port)
> + * ACA (Accessory Charger Adapters)
> + */
> +enum usb_charger_type {
> +	UNKNOWN_TYPE,
> +	SDP_TYPE,
> +	DCP_TYPE,
> +	CDP_TYPE,
> +	ACA_TYPE,
> +};
> +
> +/* USB charger state */
> +enum usb_charger_state {
> +	USB_CHARGER_DEFAULT,
> +	USB_CHARGER_PRESENT,
> +	USB_CHARGER_REMOVE,
> +};

userland really doesn't need these two ?

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
>
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.
>
> The usb charger will register notifiees on the usb gadget or the extcon
> device to get notified the usb charger state. It also supplies the
> notification mechanism to userspace When the usb charger state is changed.
>
> Power user will register a notifiee on the usb charger to get notified
> by status changes from the usb charger. It will report to power user
> to set the current limitation when detecting the usb charger is added
> or removed from extcon device state or usb gadget state.
>
> This patch doesn't yet integrate with the gadget code, so some functions
> which rely on the 'gadget' are not completed, that will be implemented
> in the following patches.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/Kconfig      |    7 +
>  drivers/usb/gadget/Makefile     |    1 +
>  drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++

It seems to me this should be part of udc-core's functionality. Maybe
move this to drivers/usb/gadget/udc and statically link it to
udc-core.ko ?

> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index af5d922..82a5b3c 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
>  	help
>  	   It supports the serial gadget can be used as a console.
>  
> +config USB_CHARGER
> +	bool "USB charger support"
> +	help
> +	  The usb charger driver based on the usb gadget that makes an
> +	  enhancement to a power driver which can set the current limitation
> +	  when the usb charger is added or removed.

sure you don't depend on anything ?

> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 598a67d..1e421c1 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
>  libcomposite-y			+= composite.o functions.o configfs.o u_f.o
>  
>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> +obj-$(CONFIG_USB_CHARGER)	+= charger.o
> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
> new file mode 100644
> index 0000000..82a9973
> --- /dev/null
> +++ b/drivers/usb/gadget/charger.c
> @@ -0,0 +1,669 @@
> +/*
> + * charger.c -- USB charger driver
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>

not very nice to depend on either of or platform_device here. What about
PCI-based devices ?

> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/usb_charger.h>
> +#include <linux/power_supply.h>
> +
> +#define DEFAULT_CUR_PROTECT	(50)

Where is this coming from ? Also, () are not necessary.

> +#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)

According to the spec we should always be talking about unit loads (1
unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
work for SS capable ports and SS gadgets (we have quite a few of them,
actually). You're missing the opportunity of charging at 900mA.

> +#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define UCHGER_STATE_LENGTH	(50)

what is this UCHGER_STATE_LENGTH ? And also, why don't you spell it out?
Is this weird name coming from a spec ? Which spec ?

> +static DEFINE_IDA(usb_charger_ida);
> +static struct bus_type usb_charger_subsys = {
> +	.name           = "usb-charger",
> +	.dev_name       = "usb-charger",
> +};
> +
> +static struct usb_charger *dev_to_uchger(struct device *udev)

nit-picking but I'd rather call this struct device *dev. Also, I'm not
sure this fits as a bus_type. There's no "usb charger" bus. There's USB
bus and its VBUS/GND lines. Why are you using a bus_type here ?

> +{
> +	return container_of(udev, struct usb_charger, dev);
> +}
> +
> +static ssize_t sdp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
> +}
> +
> +static ssize_t sdp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int sdp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &sdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(sdp_limit);

why RW ? Who's going to use these ? Also, you're not documenting this
new sysfs file.

> +static ssize_t dcp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.dcp_cur_limit);
> +}
> +
> +static ssize_t dcp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int dcp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &dcp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, DCP_TYPE, dcp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(dcp_limit);

likewise, why RW ? Missing doc.

> +static ssize_t cdp_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.cdp_cur_limit);
> +}
> +
> +static ssize_t cdp_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int cdp_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &cdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, CDP_TYPE, cdp_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(cdp_limit);

why RW? Where's doc ?

> +static ssize_t aca_limit_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return sprintf(buf, "%d\n", uchger->cur_limit.aca_cur_limit);
> +}
> +
> +static ssize_t aca_limit_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	unsigned int aca_limit;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &aca_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_charger_set_cur_limit_by_type(uchger, ACA_TYPE, aca_limit);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(aca_limit);

why RW ? where's doc ?

> +static struct attribute *usb_charger_attrs[] = {

const ?

> +	&dev_attr_sdp_limit.attr,
> +	&dev_attr_dcp_limit.attr,
> +	&dev_attr_cdp_limit.attr,
> +	&dev_attr_aca_limit.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(usb_charger);

static ?

> +struct usb_charger *usb_charger_find_by_name(const char *name)
> +{
> +	struct device *udev;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);

quite frankly, this deserves, at a minimum, a big fat WARN():

	if (WARN(!name, "can't work with NULL name"))
		return .....

> +	udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
> +	if (!udev)
> +		return ERR_PTR(-ENODEV);

still not convinced this deserves to be a bus_type.

> +	return dev_to_uchger(udev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
> +
> +/*
> + * usb_charger_get() - Reference a usb charger.
> + * @uchger - usb charger
> + */
> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
> +{
> +	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get);
> +
> +/*
> + * usb_charger_put() - Dereference a usb charger.
> + * @uchger - charger to release
> + */
> +void usb_charger_put(struct usb_charger *uchger)
> +{
> +	if (uchger)
> +		put_device(&uchger->dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_put);
> +
> +/*
> + * usb_charger_register_notify() - Register a notifiee to get notified by
> + * any attach status changes from the usb charger detection.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be registered.
> + */
> +int usb_charger_register_notify(struct usb_charger *uchger,
> +				struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)
> +		return -EINVAL;
> +
> +	mutex_lock(&uchger->lock);
> +	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
> +
> +	/* Generate an initial notify so users start in the right state */
> +	if (!ret) {
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +					usb_charger_get_cur_limit(uchger),
> +					uchger);
> +	}
> +	mutex_unlock(&uchger->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_register_notify);
> +
> +/*
> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be unregistered.
> + */
> +int usb_charger_unregister_notify(struct usb_charger *uchger,
> +				  struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)

WARN() ?

> +		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_detect_type() - Get the usb charger type by the callback
> + * which is implemented by gadget operations.
> + * @uchger - the usb charger device.
> + *
> + * return the usb charger type.
> + */
> +enum usb_charger_type
> +usb_charger_detect_type(struct usb_charger *uchger)
> +{
> +	if (uchger->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;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> +
> +/*
> + * usb_charger_set_cur_limit_by_type() - Set the current limitation
> + * by charger type.
> + * @uchger - the usb charger device.
> + * @type - the usb charger type.
> + * @cur_limit - the current limitation.
> + */
> +int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
> +				      enum usb_charger_type type,
> +				      unsigned int cur_limit)
> +{
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	switch (type) {
> +	case SDP_TYPE:
> +		uchger->cur_limit.sdp_cur_limit = cur_limit;
> +		break;
> +	case DCP_TYPE:
> +		uchger->cur_limit.dcp_cur_limit = cur_limit;
> +		break;
> +	case CDP_TYPE:
> +		uchger->cur_limit.cdp_cur_limit	= cur_limit;
> +		break;
> +	case ACA_TYPE:
> +		uchger->cur_limit.aca_cur_limit	= cur_limit;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
> +
> +/*
> + * usb_charger_set_cur_limit() - Set the current limitation.
> + * @uchger - the usb charger device.
> + * @cur_limit_set - the current limitation.
> + */
> +int usb_charger_set_cur_limit(struct usb_charger *uchger,
> +			      struct usb_charger_cur_limit *cur_limit_set)
> +{
> +	if (!uchger || !cur_limit_set)
> +		return -EINVAL;
> +
> +	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
> +	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
> +	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
> +	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit);
> +
> +/*
> + * usb_charger_get_cur_limit() - Get the current limitation by
> + * different usb charger type.
> + * @uchger - the usb charger device.
> + *
> + * return the current limitation to set.
> + */
> +unsigned int
> +usb_charger_get_cur_limit(struct usb_charger *uchger)
> +{
> +	enum usb_charger_type uchger_type = usb_charger_detect_type(uchger);
> +	unsigned int cur_limit;
> +
> +	switch (uchger_type) {
> +	case SDP_TYPE:
> +		cur_limit = uchger->cur_limit.sdp_cur_limit;
> +		break;
> +	case DCP_TYPE:
> +		cur_limit = uchger->cur_limit.dcp_cur_limit;
> +		break;
> +	case CDP_TYPE:
> +		cur_limit = uchger->cur_limit.cdp_cur_limit;
> +		break;
> +	case ACA_TYPE:
> +		cur_limit = uchger->cur_limit.aca_cur_limit;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return cur_limit;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_cur_limit);
> +
> +/*
> + * usb_charger_notifier_others() - It will notify other device registered
> + * on usb charger when the usb charger state is changed.
> + * @uchger - the usb charger device.
> + * @state - the state of the usb charger.
> + */
> +static void
> +usb_charger_notify_others(struct usb_charger *uchger,
> +			  enum usb_charger_state state)
> +{
> +	char uchger_state[UCHGER_STATE_LENGTH];
> +	char *envp[] = { uchger_state, NULL };
> +
> +	mutex_lock(&uchger->lock);
> +	uchger->state = state;
> +
> +	switch (state) {
> +	case USB_CHARGER_PRESENT:
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +			usb_charger_get_cur_limit(uchger),
> +			uchger);
> +		snprintf(uchger_state, UCHGER_STATE_LENGTH,
> +			 "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, UCHGER_STATE_LENGTH,
> +			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
> +		break;
> +	default:
> +		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
> +			 state);
> +		mutex_unlock(&uchger->lock);
> +		return;
> +	}
> +
> +	kobject_uevent_env(&uchger->dev.kobj, KOBJ_CHANGE, envp);
> +	mutex_unlock(&uchger->lock);
> +}
> +
> +/*
> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
> + * on the extcon device.
> + * @nb - the notifier block that notified by extcon device.
> + * @state - the extcon device state.
> + * @data - here specify a extcon device.
> + *
> + * return the notify flag.
> + */
> +static int
> +usb_charger_plug_by_extcon(struct notifier_block *nb,
> +			   unsigned long state, void *data)
> +{
> +	struct usb_charger_nb *extcon_nb =
> +		container_of(nb, struct usb_charger_nb, nb);
> +	struct usb_charger *uchger = extcon_nb->uchger;
> +	enum usb_charger_state uchger_state;
> +
> +	if (!uchger)
> +		return NOTIFY_BAD;
> +
> +	/* Report event to power to setting the current limitation
> +	 * for this usb charger when one usb charger is added or removed
> +	 * with detecting by extcon device.
> +	 */
> +	if (state)
> +		uchger_state = USB_CHARGER_PRESENT;
> +	else
> +		uchger_state = USB_CHARGER_REMOVE;
> +
> +	usb_charger_notify_others(uchger, uchger_state);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/*
> + * usb_charger_plug_by_gadget() - Set the usb charger current limitation
> + * according to the usb gadget device state.
> + * @gadget - the usb gadget device.
> + * @state - the usb gadget state.
> + */
> +int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
> +			       unsigned long state)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
> +
> +static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
> +{
> +	struct usb_charger **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +static void usb_charger_release(struct device *dev)
> +{
> +	struct usb_charger *uchger = dev_get_drvdata(dev);
> +
> +	kfree(uchger);
> +}
> +
> +/*
> + * usb_charger_unregister() - Unregister a usb charger device.
> + * @uchger - the usb charger to be unregistered.
> + */
> +static int usb_charger_unregister(struct usb_charger *uchger)
> +{
> +	if (!uchger)

WARN()

> +		return -EINVAL;
> +
> +	device_unregister(&uchger->dev);
> +	return 0;
> +}
> +
> +static void devm_uchger_dev_unreg(struct device *dev, void *res)
> +{
> +	usb_charger_unregister(*(struct usb_charger **)res);
> +}
> +
> +void devm_usb_charger_unregister(struct device *dev,
> +				 struct usb_charger *uchger)
> +{
> +	devres_release(dev, devm_uchger_dev_unreg,
> +		       devm_uchger_dev_match, uchger);
> +}

does this need EXPORT_SYMBOL_GPL() too ?

> +/*
> + * usb_charger_register() - Register a new usb charger device
> + * which is created by the usb charger framework.
> + * @parent - the parent device of the new usb charger.
> + * @uchger - the new usb charger device.
> + */
> +static int usb_charger_register(struct device *parent,
> +				struct usb_charger *uchger)
> +{
> +	int ret;
> +
> +	if (!uchger)

WARN()

> +		return -EINVAL;
> +
> +	uchger->dev.parent = parent;
> +	uchger->dev.release = usb_charger_release;
> +	uchger->dev.bus = &usb_charger_subsys;
> +	uchger->dev.groups = usb_charger_groups;
> +
> +	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto fail_ida;
> +
> +	uchger->id = ret;
> +	dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
> +	dev_set_drvdata(&uchger->dev, uchger);
> +
> +	ret = device_register(&uchger->dev);
> +	if (ret)
> +		goto fail_register;
> +
> +	return 0;
> +
> +fail_register:
> +	put_device(&uchger->dev);
> +	ida_simple_remove(&usb_charger_ida, uchger->id);
> +	uchger->id = -1;
> +fail_ida:
> +	dev_err(parent, "Failed to register usb charger: %d\n", ret);
> +	return ret;
> +}
> +
> +int devm_usb_charger_register(struct device *dev,
> +			      struct usb_charger *uchger)
> +{
> +	struct usb_charger **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = usb_charger_register(dev, uchger);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	*ptr = uchger;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +
> +int usb_charger_init(struct usb_gadget *ugadget)
> +{
> +	struct usb_charger *uchger;
> +	struct extcon_dev *edev;
> +	struct power_supply *psy;
> +	int ret;
> +
> +	if (!ugadget)

WARN()

but I don't like this. This should be done from udc-core.c itself when
the UDC registers itself.

> +		return -EINVAL;
> +
> +	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->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;
> +	uchger->get_charger_type = NULL;
> +
> +	mutex_init(&uchger->lock);
> +	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
> +
> +	/* register a notifier on a extcon device if it is exsited */
> +	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
> +	if (!IS_ERR_OR_NULL(edev)) {
> +		uchger->extcon_dev = edev;
> +		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
> +		uchger->extcon_nb.uchger = uchger;
> +		extcon_register_notifier(edev, EXTCON_USB,
> +					 &uchger->extcon_nb.nb);
> +	}
> +
> +	/* to check if the usb charger is link to a power supply */
> +	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
> +					       "power-supplies");
> +	if (!IS_ERR_OR_NULL(psy))
> +		uchger->psy = psy;
> +	else
> +		uchger->psy = NULL;
> +
> +	/* register a notifier on a usb gadget device */
> +	uchger->gadget = ugadget;
> +	uchger->old_gadget_state = ugadget->state;
> +
> +	/* register a new usb charger */
> +	ret = usb_charger_register(&ugadget->dev, uchger);
> +	if (ret)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	if (uchger->extcon_dev)
> +		extcon_unregister_notifier(uchger->extcon_dev,
> +					   EXTCON_USB, &uchger->extcon_nb.nb);
> +
> +	kfree(uchger);
> +	return ret;
> +}
> +
> +int usb_charger_exit(struct usb_gadget *ugadget)
> +{
> +	return 0;
> +}
> +
> +static int __init usb_charger_sysfs_init(void)
> +{
> +	return subsys_system_register(&usb_charger_subsys, NULL);
> +}
> +core_initcall(usb_charger_sysfs_init);

please don't. This should be indication that there's something wrong
with your patchset.

> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
> +MODULE_DESCRIPTION("USB charger driver");
> +MODULE_LICENSE("GPL");

GPLv2 or GPLv2+ ??

> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
> new file mode 100644
> index 0000000..eed422f
> --- /dev/null
> +++ b/include/linux/usb/usb_charger.h

usb_ is redundant. I'd prefer to:

#include <linux/usb/charger.h>

> @@ -0,0 +1,164 @@
> +#ifndef __LINUX_USB_CHARGER_H__
> +#define __LINUX_USB_CHARGER_H__
> +
> +#include <uapi/linux/usb/ch9.h>
> +
> +/* USB charger type:
> + * SDP (Standard Downstream Port)
> + * DCP (Dedicated Charging Port)
> + * CDP (Charging Downstream Port)
> + * ACA (Accessory Charger Adapters)
> + */
> +enum usb_charger_type {
> +	UNKNOWN_TYPE,
> +	SDP_TYPE,
> +	DCP_TYPE,
> +	CDP_TYPE,
> +	ACA_TYPE,
> +};
> +
> +/* USB charger state */
> +enum usb_charger_state {
> +	USB_CHARGER_DEFAULT,
> +	USB_CHARGER_PRESENT,
> +	USB_CHARGER_REMOVE,
> +};

userland really doesn't need these two ?

-- 
balbi

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

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

[Mark Brown]

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

On Wed, Mar 30, 2016 at 01:09:00PM +0300, Felipe Balbi wrote:
> Baolin Wang <baolin.wang@linaro.org> writes:

> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>

> not very nice to depend on either of or platform_device here. What about
> PCI-based devices ?

The header inclusion shouldn't be conditional though.  But looking at
the patch I can't immediately see any use of these in the code anyway.

> > +static DEVICE_ATTR_RW(sdp_limit);

> why RW ? Who's going to use these ? Also, you're not documenting this
> new sysfs file.

If they end up not writeable should we just remove them entirely since
they should just be the spec values?

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

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

[Mark Brown]

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

On Wed, Mar 30, 2016 at 01:09:00PM +0300, Felipe Balbi wrote:
> Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>

> not very nice to depend on either of or platform_device here. What about
> PCI-based devices ?

The header inclusion shouldn't be conditional though.  But looking at
the patch I can't immediately see any use of these in the code anyway.

> > +static DEVICE_ATTR_RW(sdp_limit);

> why RW ? Who's going to use these ? Also, you're not documenting this
> new sysfs file.

If they end up not writeable should we just remove them entirely since
they should just be the spec values?

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

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

[Felipe Balbi]

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

Mark Brown <broonie@kernel.org> writes:
> [ text/plain ]
> On Wed, Mar 30, 2016 at 01:09:00PM +0300, Felipe Balbi wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>
>> > +#include <linux/of.h>
>> > +#include <linux/of_device.h>
>> > +#include <linux/of_address.h>
>> > +#include <linux/platform_device.h>
>
>> not very nice to depend on either of or platform_device here. What about
>> PCI-based devices ?
>
> The header inclusion shouldn't be conditional though.  But looking at
> the patch I can't immediately see any use of these in the code anyway.

fair enough, seems like removal is the way.

>> > +static DEVICE_ATTR_RW(sdp_limit);
>
>> why RW ? Who's going to use these ? Also, you're not documenting this
>> new sysfs file.
>
> If they end up not writeable should we just remove them entirely since
> they should just be the spec values?

if they are really just spec values, why would even let them be modified
to start with ? ;-)

But yeah, seems like this is not interesting to userland.

-- 
balbi

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

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

[Baolin Wang]

On 30 March 2016 at 18:09, Felipe Balbi <balbi@kernel.org> wrote:
>> ---
>>  drivers/usb/gadget/Kconfig      |    7 +
>>  drivers/usb/gadget/Makefile     |    1 +
>>  drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++
>
> It seems to me this should be part of udc-core's functionality. Maybe
> move this to drivers/usb/gadget/udc and statically link it to
> udc-core.ko ?

OK.

>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index af5d922..82a5b3c 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
>>       help
>>          It supports the serial gadget can be used as a console.
>>
>> +config USB_CHARGER
>> +     bool "USB charger support"
>> +     help
>> +       The usb charger driver based on the usb gadget that makes an
>> +       enhancement to a power driver which can set the current limitation
>> +       when the usb charger is added or removed.
>
> sure you don't depend on anything ?

It just depends on USB_GADGET.

>
>> +
>> +#define DEFAULT_CUR_PROTECT  (50)
>
> Where is this coming from ? Also, () are not necessary.

Just want to protect the default current limitation. If that does not
need, I'll remove it.

>
>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>
> According to the spec we should always be talking about unit loads (1
> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
> work for SS capable ports and SS gadgets (we have quite a few of them,
> actually). You're missing the opportunity of charging at 900mA.

I follow the DCP/SDP/CDP/ACA type's default current limitation and
user can set them what they want.

>
>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define UCHGER_STATE_LENGTH  (50)
>
> what is this UCHGER_STATE_LENGTH ? And also, why don't you spell it out?
> Is this weird name coming from a spec ? Which spec ?

It is used to indicate the array size to save the charger state to
report to userspace. I should move it to where it is used.

>
>> +static DEFINE_IDA(usb_charger_ida);
>> +static struct bus_type usb_charger_subsys = {
>> +     .name           = "usb-charger",
>> +     .dev_name       = "usb-charger",
>> +};
>> +
>> +static struct usb_charger *dev_to_uchger(struct device *udev)
>
> nit-picking but I'd rather call this struct device *dev. Also, I'm not

OK.

> sure this fits as a bus_type. There's no "usb charger" bus. There's USB
> bus and its VBUS/GND lines. Why are you using a bus_type here ?

I want to use bus structure to manage the charger device. Maybe choose
class to manage them?

>
>> +{
>> +     return container_of(udev, struct usb_charger, dev);
>> +}
>> +
>> +static ssize_t sdp_limit_show(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +
>> +     return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
>> +}
>> +
>> +static ssize_t sdp_limit_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t count)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +     unsigned int sdp_limit;
>> +     int ret;
>> +
>> +     ret = kstrtouint(buf, 10, &sdp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +static DEVICE_ATTR_RW(sdp_limit);
>
> why RW ? Who's going to use these ? Also, you're not documenting this
> new sysfs file.

Cause we have show and store operation for SDP type. If users want to
know or set the SDP current, they can use the sysfs file.
I'll add the documentation for it.

>
>> +static ssize_t dcp_limit_show(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +
>> +     return sprintf(buf, "%d\n", uchger->cur_limit.dcp_cur_limit);
>> +}
>> +
>> +static ssize_t dcp_limit_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t count)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +     unsigned int dcp_limit;
>> +     int ret;
>> +
>> +     ret = kstrtouint(buf, 10, &dcp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = usb_charger_set_cur_limit_by_type(uchger, DCP_TYPE, dcp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +static DEVICE_ATTR_RW(dcp_limit);
>
> likewise, why RW ? Missing doc.

Same reason. I'll add the doc.

>
>> +static ssize_t cdp_limit_show(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +
>> +     return sprintf(buf, "%d\n", uchger->cur_limit.cdp_cur_limit);
>> +}
>> +
>> +static ssize_t cdp_limit_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t count)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +     unsigned int cdp_limit;
>> +     int ret;
>> +
>> +     ret = kstrtouint(buf, 10, &cdp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = usb_charger_set_cur_limit_by_type(uchger, CDP_TYPE, cdp_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +static DEVICE_ATTR_RW(cdp_limit);
>
> why RW? Where's doc ?

I'll add the doc in next version.

>
>> +static ssize_t aca_limit_show(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +
>> +     return sprintf(buf, "%d\n", uchger->cur_limit.aca_cur_limit);
>> +}
>> +
>> +static ssize_t aca_limit_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t count)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +     unsigned int aca_limit;
>> +     int ret;
>> +
>> +     ret = kstrtouint(buf, 10, &aca_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = usb_charger_set_cur_limit_by_type(uchger, ACA_TYPE, aca_limit);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +static DEVICE_ATTR_RW(aca_limit);
>
> why RW ? where's doc ?

I'll add the doc in next version.

>
>> +static struct attribute *usb_charger_attrs[] = {
>
> const ?

OK.

>
>> +     &dev_attr_sdp_limit.attr,
>> +     &dev_attr_dcp_limit.attr,
>> +     &dev_attr_cdp_limit.attr,
>> +     &dev_attr_aca_limit.attr,
>> +     NULL
>> +};
>> +ATTRIBUTE_GROUPS(usb_charger);
>
> static ?

This macro will add 'static' automatically. So don't need to add.

>
>> +struct usb_charger *usb_charger_find_by_name(const char *name)
>> +{
>> +     struct device *udev;
>> +
>> +     if (!name)
>> +             return ERR_PTR(-EINVAL);
>
> quite frankly, this deserves, at a minimum, a big fat WARN():
>
>         if (WARN(!name, "can't work with NULL name"))
>                 return .....

OK.

>
>> +     udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
>> +     if (!udev)
>> +             return ERR_PTR(-ENODEV);
>
> still not convinced this deserves to be a bus_type.

I'll re-consider about the bus type things.

>> +int usb_charger_unregister_notify(struct usb_charger *uchger,
>> +                               struct notifier_block *nb)
>> +{
>> +     int ret;
>> +
>> +     if (!uchger || !nb)
>
> WARN() ?

OK.


>> +static int usb_charger_unregister(struct usb_charger *uchger)
>> +{
>> +     if (!uchger)
>
> WARN()

OK.

>
>> +             return -EINVAL;
>> +
>> +     device_unregister(&uchger->dev);
>> +     return 0;
>> +}
>> +
>> +static void devm_uchger_dev_unreg(struct device *dev, void *res)
>> +{
>> +     usb_charger_unregister(*(struct usb_charger **)res);
>> +}
>> +
>> +void devm_usb_charger_unregister(struct device *dev,
>> +                              struct usb_charger *uchger)
>> +{
>> +     devres_release(dev, devm_uchger_dev_unreg,
>> +                    devm_uchger_dev_match, uchger);
>> +}
>
> does this need EXPORT_SYMBOL_GPL() too ?

OK. I'll add it.

>
>> +/*
>> + * usb_charger_register() - Register a new usb charger device
>> + * which is created by the usb charger framework.
>> + * @parent - the parent device of the new usb charger.
>> + * @uchger - the new usb charger device.
>> + */
>> +static int usb_charger_register(struct device *parent,
>> +                             struct usb_charger *uchger)
>> +{
>> +     int ret;
>> +
>> +     if (!uchger)
>
> WARN()

OK.

>
>> +int usb_charger_init(struct usb_gadget *ugadget)
>> +{
>> +     struct usb_charger *uchger;
>> +     struct extcon_dev *edev;
>> +     struct power_supply *psy;
>> +     int ret;
>> +
>> +     if (!ugadget)
>
> WARN()

OK.

>
> but I don't like this. This should be done from udc-core.c itself when
> the UDC registers itself.

Make sense.

>
>> +
>> +static int __init usb_charger_sysfs_init(void)
>> +{
>> +     return subsys_system_register(&usb_charger_subsys, NULL);
>> +}
>> +core_initcall(usb_charger_sysfs_init);
>
> please don't. This should be indication that there's something wrong
> with your patchset.

I'll modify the bus things.

>
>> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
>> +MODULE_DESCRIPTION("USB charger driver");
>> +MODULE_LICENSE("GPL");
>
> GPLv2 or GPLv2+ ??

OK.

>
>> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
>> new file mode 100644
>> index 0000000..eed422f
>> --- /dev/null
>> +++ b/include/linux/usb/usb_charger.h
>
> usb_ is redundant. I'd prefer to:
>
> #include <linux/usb/charger.h>

Got it.

>
>> +
>> +/* USB charger state */
>> +enum usb_charger_state {
>> +     USB_CHARGER_DEFAULT,
>> +     USB_CHARGER_PRESENT,
>> +     USB_CHARGER_REMOVE,
>> +};
>
> userland really doesn't need these two ?

We've reported to userspace by kobject_uevent in
'usb_charger_notify_others()' function.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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

Baolin Wang <baolin.wang@linaro.org> writes:
>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>> index af5d922..82a5b3c 100644
>>> --- a/drivers/usb/gadget/Kconfig
>>> +++ b/drivers/usb/gadget/Kconfig
>>> @@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
>>>       help
>>>          It supports the serial gadget can be used as a console.
>>>
>>> +config USB_CHARGER
>>> +     bool "USB charger support"
>>> +     help
>>> +       The usb charger driver based on the usb gadget that makes an
>>> +       enhancement to a power driver which can set the current limitation
>>> +       when the usb charger is added or removed.
>>
>> sure you don't depend on anything ?
>
> It just depends on USB_GADGET.

okay, that's cool :-)

>>> +
>>> +#define DEFAULT_CUR_PROTECT  (50)
>>
>> Where is this coming from ? Also, () are not necessary.
>
> Just want to protect the default current limitation. If that does not
> need, I'll remove it.

It's your HW :-) You tell me if it's really necessary. But, hey, if you
get enumerated @500mA, this is the host telling you it _CAN_ give you
500mA. In that case, why wouldn't you ?

>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>
>> According to the spec we should always be talking about unit loads (1
>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>> work for SS capable ports and SS gadgets (we have quite a few of them,
>> actually). You're missing the opportunity of charging at 900mA.
>
> I follow the DCP/SDP/CDP/ACA type's default current limitation and
> user can set them what they want.

no, the user CANNOT set it to what they want. If you get enumerated
@100mA and the user just decides to set it to 2000mA, s/he could even
melt the USB connector. The kernel _must_ prevent such cases.

In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
variable because if you enumerate in SS, you _can_ get up to 900mA.

>>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>> +#define UCHGER_STATE_LENGTH  (50)
>>
>> what is this UCHGER_STATE_LENGTH ? And also, why don't you spell it out?
>> Is this weird name coming from a spec ? Which spec ?
>
> It is used to indicate the array size to save the charger state to
> report to userspace. I should move it to where it is used.

and ARRAY_SIZE(arr) is not enough ?

>>> +static DEFINE_IDA(usb_charger_ida);
>>> +static struct bus_type usb_charger_subsys = {
>>> +     .name           = "usb-charger",
>>> +     .dev_name       = "usb-charger",
>>> +};
>>> +
>>> +static struct usb_charger *dev_to_uchger(struct device *udev)
>>
>> nit-picking but I'd rather call this struct device *dev. Also, I'm not
>
> OK.
>
>> sure this fits as a bus_type. There's no "usb charger" bus. There's USB
>> bus and its VBUS/GND lines. Why are you using a bus_type here ?
>
> I want to use bus structure to manage the charger device. Maybe choose
> class to manage them?

I guess a class would fit better in this case.

>>> +{
>>> +     return container_of(udev, struct usb_charger, dev);
>>> +}
>>> +
>>> +static ssize_t sdp_limit_show(struct device *dev,
>>> +                           struct device_attribute *attr,
>>> +                           char *buf)
>>> +{
>>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>>> +
>>> +     return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
>>> +}
>>> +
>>> +static ssize_t sdp_limit_store(struct device *dev,
>>> +                            struct device_attribute *attr,
>>> +                            const char *buf, size_t count)
>>> +{
>>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>>> +     unsigned int sdp_limit;
>>> +     int ret;
>>> +
>>> +     ret = kstrtouint(buf, 10, &sdp_limit);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return count;
>>> +}
>>> +static DEVICE_ATTR_RW(sdp_limit);
>>
>> why RW ? Who's going to use these ? Also, you're not documenting this
>> new sysfs file.
>
> Cause we have show and store operation for SDP type. If users want to
> know or set the SDP current, they can use the sysfs file.
> I'll add the documentation for it.

but why would the user change it ? Here's the thing: you have a few
posibilities for this:

a) you are connected to a dedicated charger

	In this case, you can get up to 2000mA depending on the charger.

	If $this charger can give you or not 2000mA is not detectable,
	so what do charging ICs do ? They slowly increase the attached
	load accross VBUS/GND and measure VBUS value. When IC notices
	VBUS dropping bit, step back to previous load.

	This means you will always charger with maximum rating of DCP.

	Why would user change this ? More is unsafe, less is just
	stupid.

b) you are connected to a host charging port and get enumerated with
your 500mA configuration.

	you *know* 500mA is okay, but you _can_ get more (it is a
	charging port after all). So charging IC will connect a 500mA
	load and step upwards until VBUS drops a little, just like (a)
	above.
        
	This means you will always charger with maximum rating for this
	host charging port.

	Why would user change this ? More is unsafe, less is just
	stupid.

c) you are connected to a standard port and get enumerated with your
500mA configuration.

        you *know* 500mA is okay. So you connect a 500mA load and get it
        over with.

	This means you will always charger with maximum rating for this
	SDP.

	Why would user change this ? More is unsafe, less is just
	stupid.

d) you are connected to a standard port and get enumerated with your
100mA configuration.

	you *know* 100mA is okay. So you connect a 100mA load and get it
	over with.

	This means you will always charger with maximum rating for this
	SDP.

	Why would user change this ? More is unsafe, less is just
	stupid.

do you see what I mean ? It's pointless to let this
be-writeable. Whatever value user writes will either be unsafe or
sub-optimal.

Just trust the charging IC to do a good job.

>>> +     &dev_attr_sdp_limit.attr,
>>> +     &dev_attr_dcp_limit.attr,
>>> +     &dev_attr_cdp_limit.attr,
>>> +     &dev_attr_aca_limit.attr,
>>> +     NULL
>>> +};
>>> +ATTRIBUTE_GROUPS(usb_charger);
>>
>> static ?
>
> This macro will add 'static' automatically. So don't need to add.

indeed, thanks.

>>> +
>>> +/* USB charger state */
>>> +enum usb_charger_state {
>>> +     USB_CHARGER_DEFAULT,
>>> +     USB_CHARGER_PRESENT,
>>> +     USB_CHARGER_REMOVE,
>>> +};
>>
>> userland really doesn't need these two ?
>
> We've reported to userspace by kobject_uevent in
> 'usb_charger_notify_others()' function.

I mean as a type ;-) So userspace doesn't have to redefine these for
their applications.

-- 
balbi

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

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

[Pavel Machek]

Hi!

> >>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
> >>
> >> According to the spec we should always be talking about unit loads (1
> >> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
> >> work for SS capable ports and SS gadgets (we have quite a few of them,
> >> actually). You're missing the opportunity of charging at 900mA.
> >
> > I follow the DCP/SDP/CDP/ACA type's default current limitation and
> > user can set them what they want.
> 
> no, the user CANNOT set it to what they want. If you get enumerated
> @100mA and the user just decides to set it to 2000mA, s/he could even
> melt the USB connector. The kernel _must_ prevent such cases.

root should be allowed to do that.

Very often, you want to charge using 1.8A from an old desktop PC.

N900 will simply not charge from .5A.

> a) you are connected to a dedicated charger
> 
> 	In this case, you can get up to 2000mA depending on the charger.
> 
> 	If $this charger can give you or not 2000mA is not detectable,
> 	so what do charging ICs do ? They slowly increase the attached
> 	load accross VBUS/GND and measure VBUS value. When IC notices
> 	VBUS dropping bit, step back to previous load.
> 
> 	This means you will always charger with maximum rating of DCP.
> 
> 	Why would user change this ? More is unsafe, less is just
> 	stupid.

Actually, less is not stupid. Charging li-ion battery from li-ion battery might
be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
(3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
energy. If I limit current to approximately the power consumption, it will run the
powerbank empty, first, then empty the internal battery, maximizing total time I
can use the device.

Best regards,
										Pavel

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> >>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>> >>
>> >> According to the spec we should always be talking about unit loads (1
>> >> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>> >> work for SS capable ports and SS gadgets (we have quite a few of them,
>> >> actually). You're missing the opportunity of charging at 900mA.
>> >
>> > I follow the DCP/SDP/CDP/ACA type's default current limitation and
>> > user can set them what they want.
>> 
>> no, the user CANNOT set it to what they want. If you get enumerated
>> @100mA and the user just decides to set it to 2000mA, s/he could even
>> melt the USB connector. The kernel _must_ prevent such cases.
>
> root should be allowed to do that.

root should not be allowed to put user at risk. The usb connector is a
path to earth ground in most (all?) desktop computers. Allowing root to
put that connector in a situation where it could melt the connector and
short mains should never be allowed.

> Very often, you want to charge using 1.8A from an old desktop PC.

if that old desktop's port is not a charging port, you shouldn't be
allowed to do that. Not ever.

> N900 will simply not charge from .5A.

it used to with original maemo userspace/kernel.

>> a) you are connected to a dedicated charger
>> 
>> 	In this case, you can get up to 2000mA depending on the charger.
>> 
>> 	If $this charger can give you or not 2000mA is not detectable,
>> 	so what do charging ICs do ? They slowly increase the attached
>> 	load accross VBUS/GND and measure VBUS value. When IC notices
>> 	VBUS dropping bit, step back to previous load.
>> 
>> 	This means you will always charger with maximum rating of DCP.
>> 
>> 	Why would user change this ? More is unsafe, less is just
>> 	stupid.
>
> Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> energy. If I limit current to approximately the power consumption, it will run the
> powerbank empty, first, then empty the internal battery, maximizing total time I
> can use the device.

why would you waste energy ? What the charger chip would do is charge
battery to maximum then just to maintenance charge from that point
on. Where is energy being wasted other than normal heat dissipation ?

-- 
balbi

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

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

[Pavel Machek]

Hi!

> >> >>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
> >> >>
> >> >> According to the spec we should always be talking about unit loads (1
> >> >> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
> >> >> work for SS capable ports and SS gadgets (we have quite a few of them,
> >> >> actually). You're missing the opportunity of charging at 900mA.
> >> >
> >> > I follow the DCP/SDP/CDP/ACA type's default current limitation and
> >> > user can set them what they want.
> >> 
> >> no, the user CANNOT set it to what they want. If you get enumerated
> >> @100mA and the user just decides to set it to 2000mA, s/he could even
> >> melt the USB connector. The kernel _must_ prevent such cases.
> >
> > root should be allowed to do that.
> 
> root should not be allowed to put user at risk. The usb connector is a
> path to earth ground in most (all?) desktop computers. Allowing root to
> put that connector in a situation where it could melt the connector and
> short mains should never be allowed.

Be sure that hardware people put reasonable precautions...

> > Very often, you want to charge using 1.8A from an old desktop PC.
> 
> if that old desktop's port is not a charging port, you shouldn't be
> allowed to do that. Not ever.

Yes, Felipe just decided that I should not be able to charge my N900
in useful way.

> > N900 will simply not charge from .5A.
> 
> it used to with original maemo userspace/kernel.

Yeah, at about 10% per night.

> >> a) you are connected to a dedicated charger
> >> 
> >> 	In this case, you can get up to 2000mA depending on the charger.
> >> 
> >> 	If $this charger can give you or not 2000mA is not detectable,
> >> 	so what do charging ICs do ? They slowly increase the attached
> >> 	load accross VBUS/GND and measure VBUS value. When IC notices
> >> 	VBUS dropping bit, step back to previous load.
> >> 
> >> 	This means you will always charger with maximum rating of DCP.
> >> 
> >> 	Why would user change this ? More is unsafe, less is just
> >> 	stupid.
> >
> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> > energy. If I limit current to approximately the power consumption, it will run the
> > powerbank empty, first, then empty the internal battery, maximizing total time I
> > can use the device.
> 
> why would you waste energy ? What the charger chip would do is charge
> battery to maximum then just to maintenance charge from that point
> on. Where is energy being wasted other than normal heat dissipation ?

Physics 101, of course wasted energy goes to heat. Lets not waste
energy by charging li-ion from li-ion when it is not required.

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> > Very often, you want to charge using 1.8A from an old desktop PC.
>> 
>> if that old desktop's port is not a charging port, you shouldn't be
>> allowed to do that. Not ever.
>
> Yes, Felipe just decided that I should not be able to charge my N900
> in useful way.

you can do whatever you want with *your* kernel binary, but we're not
gonna ship something potentially dangerous. If that PC port is telling
you it can only allow 100mA, you should *not* be allowed to overcome
that limitation from the device side, sorry.

>> >> a) you are connected to a dedicated charger
>> >> 
>> >> 	In this case, you can get up to 2000mA depending on the charger.
>> >> 
>> >> 	If $this charger can give you or not 2000mA is not detectable,
>> >> 	so what do charging ICs do ? They slowly increase the attached
>> >> 	load accross VBUS/GND and measure VBUS value. When IC notices
>> >> 	VBUS dropping bit, step back to previous load.
>> >> 
>> >> 	This means you will always charger with maximum rating of DCP.
>> >> 
>> >> 	Why would user change this ? More is unsafe, less is just
>> >> 	stupid.
>> >
>> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
>> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
>> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
>> > energy. If I limit current to approximately the power consumption, it will run the
>> > powerbank empty, first, then empty the internal battery, maximizing total time I
>> > can use the device.
>> 
>> why would you waste energy ? What the charger chip would do is charge
>> battery to maximum then just to maintenance charge from that point
>> on. Where is energy being wasted other than normal heat dissipation ?
>
> Physics 101, of course wasted energy goes to heat. Lets not waste
> energy by charging li-ion from li-ion when it is not required.

your cellphone has no means to know that it's connected to a Li-Ion
battery. We don't have visibility on what we're connected to, just how
much it can source.

-- 
balbi

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

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

[Pavel Machek]

On Mon 2016-04-18 13:30:54, Felipe Balbi wrote:
> 
> Hi,
> 
> Pavel Machek <pavel@ucw.cz> writes:
> >> > Very often, you want to charge using 1.8A from an old desktop PC.
> >> 
> >> if that old desktop's port is not a charging port, you shouldn't be
> >> allowed to do that. Not ever.
> >
> > Yes, Felipe just decided that I should not be able to charge my N900
> > in useful way.
> 
> you can do whatever you want with *your* kernel binary, but we're not
> gonna ship something potentially dangerous. If that PC port is telling
> you it can only allow 100mA, you should *not* be allowed to overcome
> that limitation from the device side, sorry.

Yes, and if your cpu tells you it can only run on 2GHz, you should not
be able to run it on 2.1GHz. And you should not be able to use
non-VESA VGA modes, because you could overheat coils in the monitor.

You are shipping something potentially dangerous already, because you
know, USB-A-to-micro-B cables with D+/D- connected to simulate charger
are actually very common. The world did not end, yet, so it is clearly
not as bad as you make it be.

> >> >> a) you are connected to a dedicated charger
> >> >> 
> >> >> 	In this case, you can get up to 2000mA depending on the charger.
> >> >> 
> >> >> 	If $this charger can give you or not 2000mA is not detectable,
> >> >> 	so what do charging ICs do ? They slowly increase the attached
> >> >> 	load accross VBUS/GND and measure VBUS value. When IC notices
> >> >> 	VBUS dropping bit, step back to previous load.
> >> >> 
> >> >> 	This means you will always charger with maximum rating of DCP.
> >> >> 
> >> >> 	Why would user change this ? More is unsafe, less is just
> >> >> 	stupid.
> >> >
> >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> >> > energy. If I limit current to approximately the power consumption, it will run the
> >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> >> > can use the device.
> >> 
> >> why would you waste energy ? What the charger chip would do is charge
> >> battery to maximum then just to maintenance charge from that point
> >> on. Where is energy being wasted other than normal heat dissipation ?
> >
> > Physics 101, of course wasted energy goes to heat. Lets not waste
> > energy by charging li-ion from li-ion when it is not required.
> 
> your cellphone has no means to know that it's connected to a Li-Ion
> battery. We don't have visibility on what we're connected to, just how
> much it can source.

But cellphone user knows what he connected his charger to, and that's
why it is useful to be able to lower the current. Even when you said
"less is just stupid" I demonstrated it is not, at least in case when
your power source is a battery.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Pavel Machek]

On Mon 2016-04-18 13:30:54, Felipe Balbi wrote:
> 
> Hi,
> 
> Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> writes:
> >> > Very often, you want to charge using 1.8A from an old desktop PC.
> >> 
> >> if that old desktop's port is not a charging port, you shouldn't be
> >> allowed to do that. Not ever.
> >
> > Yes, Felipe just decided that I should not be able to charge my N900
> > in useful way.
> 
> you can do whatever you want with *your* kernel binary, but we're not
> gonna ship something potentially dangerous. If that PC port is telling
> you it can only allow 100mA, you should *not* be allowed to overcome
> that limitation from the device side, sorry.

Yes, and if your cpu tells you it can only run on 2GHz, you should not
be able to run it on 2.1GHz. And you should not be able to use
non-VESA VGA modes, because you could overheat coils in the monitor.

You are shipping something potentially dangerous already, because you
know, USB-A-to-micro-B cables with D+/D- connected to simulate charger
are actually very common. The world did not end, yet, so it is clearly
not as bad as you make it be.

> >> >> a) you are connected to a dedicated charger
> >> >> 
> >> >> 	In this case, you can get up to 2000mA depending on the charger.
> >> >> 
> >> >> 	If $this charger can give you or not 2000mA is not detectable,
> >> >> 	so what do charging ICs do ? They slowly increase the attached
> >> >> 	load accross VBUS/GND and measure VBUS value. When IC notices
> >> >> 	VBUS dropping bit, step back to previous load.
> >> >> 
> >> >> 	This means you will always charger with maximum rating of DCP.
> >> >> 
> >> >> 	Why would user change this ? More is unsafe, less is just
> >> >> 	stupid.
> >> >
> >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> >> > energy. If I limit current to approximately the power consumption, it will run the
> >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> >> > can use the device.
> >> 
> >> why would you waste energy ? What the charger chip would do is charge
> >> battery to maximum then just to maintenance charge from that point
> >> on. Where is energy being wasted other than normal heat dissipation ?
> >
> > Physics 101, of course wasted energy goes to heat. Lets not waste
> > energy by charging li-ion from li-ion when it is not required.
> 
> your cellphone has no means to know that it's connected to a Li-Ion
> battery. We don't have visibility on what we're connected to, just how
> much it can source.

But cellphone user knows what he connected his charger to, and that's
why it is useful to be able to lower the current. Even when you said
"less is just stupid" I demonstrated it is not, at least in case when
your power source is a battery.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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 v7 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
> On Mon 2016-04-18 13:30:54, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Pavel Machek <pavel@ucw.cz> writes:
>> >> > Very often, you want to charge using 1.8A from an old desktop PC.
>> >> 
>> >> if that old desktop's port is not a charging port, you shouldn't be
>> >> allowed to do that. Not ever.
>> >
>> > Yes, Felipe just decided that I should not be able to charge my N900
>> > in useful way.
>> 
>> you can do whatever you want with *your* kernel binary, but we're not
>> gonna ship something potentially dangerous. If that PC port is telling
>> you it can only allow 100mA, you should *not* be allowed to overcome
>> that limitation from the device side, sorry.
>
> Yes, and if your cpu tells you it can only run on 2GHz, you should not
> be able to run it on 2.1GHz. And you should not be able to use
> non-VESA VGA modes, because you could overheat coils in the monitor.

heh, yada-yada-yada.

> You are shipping something potentially dangerous already, because you
> know, USB-A-to-micro-B cables with D+/D- connected to simulate charger
> are actually very common. The world did not end, yet, so it is clearly
> not as bad as you make it be.

That's not even a supported cable assembly. If you break your HW, that's
your own fault for not using proper cables. There's nothing the kernel
can do about that anyway.

If you're willing to by a "special" cable just to overcome safety limits
set by the various USB specifications, go head. But don't ask me to
support you when things go wrong.

>> >> >> a) you are connected to a dedicated charger
>> >> >> 
>> >> >> 	In this case, you can get up to 2000mA depending on the charger.
>> >> >> 
>> >> >> 	If $this charger can give you or not 2000mA is not detectable,
>> >> >> 	so what do charging ICs do ? They slowly increase the attached
>> >> >> 	load accross VBUS/GND and measure VBUS value. When IC notices
>> >> >> 	VBUS dropping bit, step back to previous load.
>> >> >> 
>> >> >> 	This means you will always charger with maximum rating of DCP.
>> >> >> 
>> >> >> 	Why would user change this ? More is unsafe, less is just
>> >> >> 	stupid.
>> >> >
>> >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
>> >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
>> >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
>> >> > energy. If I limit current to approximately the power consumption, it will run the
>> >> > powerbank empty, first, then empty the internal battery, maximizing total time I
>> >> > can use the device.
>> >> 
>> >> why would you waste energy ? What the charger chip would do is charge
>> >> battery to maximum then just to maintenance charge from that point
>> >> on. Where is energy being wasted other than normal heat dissipation ?
>> >
>> > Physics 101, of course wasted energy goes to heat. Lets not waste
>> > energy by charging li-ion from li-ion when it is not required.
>> 
>> your cellphone has no means to know that it's connected to a Li-Ion
>> battery. We don't have visibility on what we're connected to, just how
>> much it can source.
>
> But cellphone user knows what he connected his charger to, and that's
> why it is useful to be able to lower the current. Even when you said
> "less is just stupid" I demonstrated it is not, at least in case when
> your power source is a battery.

okay, so let's do this. How much more "time" do you get by doing this ?
Without numbers showing that it's considerably better, we can't do
anything.

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Felipe Balbi <balbi@kernel.org> writes:
>> But cellphone user knows what he connected his charger to, and that's
>> why it is useful to be able to lower the current. Even when you said
>> "less is just stupid" I demonstrated it is not, at least in case when

and btw, you haven't demonstrated anything. You merely stated that it
isn't without references or numbers, or any source of trustworthy
information. I'm not really into 'believing'.

-- 
balbi

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

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

[Pavel Machek]

On Mon 2016-04-18 13:55:17, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Balbi <balbi@kernel.org> writes:
> >> But cellphone user knows what he connected his charger to, and that's
> >> why it is useful to be able to lower the current. Even when you said
> >> "less is just stupid" I demonstrated it is not, at least in case when
> 
> and btw, you haven't demonstrated anything. You merely stated that it
> isn't without references or numbers, or any source of trustworthy
> information. I'm not really into 'believing'.

You are not really into reading, either, it seems. Or into the
electronics. Or into physics.

You seem to understand that charging li-ion from li-ion produces too
much heat. (And yes, it does, DC-DC convertors are not 100%
effective). Converting energy into heat is not a good idea.

If you need numbers or references to understand basic physics, you'll
need to google it yourself, I'm afraid. 

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
> On Mon 2016-04-18 13:55:17, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Felipe Balbi <balbi@kernel.org> writes:
>> >> But cellphone user knows what he connected his charger to, and that's
>> >> why it is useful to be able to lower the current. Even when you said
>> >> "less is just stupid" I demonstrated it is not, at least in case when
>> 
>> and btw, you haven't demonstrated anything. You merely stated that it
>> isn't without references or numbers, or any source of trustworthy
>> information. I'm not really into 'believing'.
>
> You are not really into reading, either, it seems. Or into the
> electronics. Or into physics.
>
> You seem to understand that charging li-ion from li-ion produces too
> much heat. (And yes, it does, DC-DC convertors are not 100%
> effective). Converting energy into heat is not a good idea.
>
> If you need numbers or references to understand basic physics, you'll
> need to google it yourself, I'm afraid. 

still doesn't change the fact that you haven't demonstrated anything. If
you can't be helpful and/or constructive, you may also go away and use
whatever unsafe setup you want to use.

-- 
balbi

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

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

[Pavel Machek]

On Mon 2016-04-18 14:42:58, Felipe Balbi wrote:
> 
> Hi,
> 
> Pavel Machek <pavel@ucw.cz> writes:
> > On Mon 2016-04-18 13:55:17, Felipe Balbi wrote:
> >> 
> >> Hi,
> >> 
> >> Felipe Balbi <balbi@kernel.org> writes:
> >> >> But cellphone user knows what he connected his charger to, and that's
> >> >> why it is useful to be able to lower the current. Even when you said
> >> >> "less is just stupid" I demonstrated it is not, at least in case when
> >> 
> >> and btw, you haven't demonstrated anything. You merely stated that it
> >> isn't without references or numbers, or any source of trustworthy
> >> information. I'm not really into 'believing'.
> >
> > You are not really into reading, either, it seems. Or into the
> > electronics. Or into physics.
> >
> > You seem to understand that charging li-ion from li-ion produces too
> > much heat. (And yes, it does, DC-DC convertors are not 100%
> > effective). Converting energy into heat is not a good idea.
> >
> > If you need numbers or references to understand basic physics, you'll
> > need to google it yourself, I'm afraid. 
> 
> still doesn't change the fact that you haven't demonstrated anything. If
> you can't be helpful and/or constructive, you may also go away and use
> whatever unsafe setup you want to use.

So you still claim that it is "stupid" to charge at anything lower
than maximum allowed current?

Even after I described the example with device running off power bank
on a train? Would you explain your reasoning?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> Pavel Machek <pavel@ucw.cz> writes:
>> > On Mon 2016-04-18 13:55:17, Felipe Balbi wrote:
>> >> 
>> >> Hi,
>> >> 
>> >> Felipe Balbi <balbi@kernel.org> writes:
>> >> >> But cellphone user knows what he connected his charger to, and that's
>> >> >> why it is useful to be able to lower the current. Even when you said
>> >> >> "less is just stupid" I demonstrated it is not, at least in case when
>> >> 
>> >> and btw, you haven't demonstrated anything. You merely stated that it
>> >> isn't without references or numbers, or any source of trustworthy
>> >> information. I'm not really into 'believing'.
>> >
>> > You are not really into reading, either, it seems. Or into the
>> > electronics. Or into physics.
>> >
>> > You seem to understand that charging li-ion from li-ion produces too
>> > much heat. (And yes, it does, DC-DC convertors are not 100%
>> > effective). Converting energy into heat is not a good idea.
>> >
>> > If you need numbers or references to understand basic physics, you'll
>> > need to google it yourself, I'm afraid. 
>> 
>> still doesn't change the fact that you haven't demonstrated anything. If
>> you can't be helpful and/or constructive, you may also go away and use
>> whatever unsafe setup you want to use.
>
> So you still claim that it is "stupid" to charge at anything lower
> than maximum allowed current?
>
> Even after I described the example with device running off power bank
> on a train? Would you explain your reasoning?

you still haven't produced evidence of your claim. Claiming without
evidence has virtually no meaning. Anyway, I'll just google a bit later
seen as if you're just unwilling to produce evidence of your claims.

-- 
balbi

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

RE: [PATCH v7 1/4] gadget: Introduce the usb charger framework

[David Laight]

From: Pavel Machek
> Sent: 18 April 2016 11:40
...
> > >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> > >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> > >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> > >> > energy. If I limit current to approximately the power consumption, it will run the
> > >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> > >> > can use the device.
> > >>
> > >> why would you waste energy ? What the charger chip would do is charge
> > >> battery to maximum then just to maintenance charge from that point
> > >> on. Where is energy being wasted other than normal heat dissipation ?
> > >
> > > Physics 101, of course wasted energy goes to heat. Lets not waste
> > > energy by charging li-ion from li-ion when it is not required.
> >
> > your cellphone has no means to know that it's connected to a Li-Ion
> > battery. We don't have visibility on what we're connected to, just how
> > much it can source.
> 
> But cellphone user knows what he connected his charger to, and that's
> why it is useful to be able to lower the current. Even when you said
> "less is just stupid" I demonstrated it is not, at least in case when
> your power source is a battery.

It reality you may want the phone/tablet to be configurable to take power
from USB, but disable the li-ion charging circuit.
That will maximise the time you get when running from an external battery.
I connect my tablet to the 1A output (which discharges the internal battery
slowly) rather than the 2A one (which will charge it with some cables).

Getting 2A from a USB charger seems to be very device/cable/charger dependant.
We've two Samsung chargers that look similar, but have completely different
characteristics. Both claim 2A at 5v (one says 5.3v), one claims 1.5A at
something like 12v as well - not sure we should be using that one for
'random' devices.

	David

RE: [PATCH v7 1/4] gadget: Introduce the usb charger framework

[David Laight]

From: Pavel Machek
> Sent: 18 April 2016 11:40
...
> > >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> > >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> > >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> > >> > energy. If I limit current to approximately the power consumption, it will run the
> > >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> > >> > can use the device.
> > >>
> > >> why would you waste energy ? What the charger chip would do is charge
> > >> battery to maximum then just to maintenance charge from that point
> > >> on. Where is energy being wasted other than normal heat dissipation ?
> > >
> > > Physics 101, of course wasted energy goes to heat. Lets not waste
> > > energy by charging li-ion from li-ion when it is not required.
> >
> > your cellphone has no means to know that it's connected to a Li-Ion
> > battery. We don't have visibility on what we're connected to, just how
> > much it can source.
> 
> But cellphone user knows what he connected his charger to, and that's
> why it is useful to be able to lower the current. Even when you said
> "less is just stupid" I demonstrated it is not, at least in case when
> your power source is a battery.

It reality you may want the phone/tablet to be configurable to take power
from USB, but disable the li-ion charging circuit.
That will maximise the time you get when running from an external battery.
I connect my tablet to the 1A output (which discharges the internal battery
slowly) rather than the 2A one (which will charge it with some cables).

Getting 2A from a USB charger seems to be very device/cable/charger dependant.
We've two Samsung chargers that look similar, but have completely different
characteristics. Both claim 2A at 5v (one says 5.3v), one claims 1.5A at
something like 12v as well - not sure we should be using that one for
'random' devices.

	David

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

[Pavel Machek]

Hi!

On Mon 2016-04-18 10:59:23, David Laight wrote:
> From: Pavel Machek
> > Sent: 18 April 2016 11:40
> ...
> > > >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> > > >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> > > >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> > > >> > energy. If I limit current to approximately the power consumption, it will run the
> > > >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> > > >> > can use the device.
> > > >>
> > > >> why would you waste energy ? What the charger chip would do is charge
> > > >> battery to maximum then just to maintenance charge from that point
> > > >> on. Where is energy being wasted other than normal heat dissipation ?
> > > >
> > > > Physics 101, of course wasted energy goes to heat. Lets not waste
> > > > energy by charging li-ion from li-ion when it is not required.
> > >
> > > your cellphone has no means to know that it's connected to a Li-Ion
> > > battery. We don't have visibility on what we're connected to, just how
> > > much it can source.
> > 
> > But cellphone user knows what he connected his charger to, and that's
> > why it is useful to be able to lower the current. Even when you said
> > "less is just stupid" I demonstrated it is not, at least in case when
> > your power source is a battery.
> 
> It reality you may want the phone/tablet to be configurable to take power
> from USB, but disable the li-ion charging circuit.
> That will maximise the time you get when running from an external battery.
> I connect my tablet to the 1A output (which discharges the internal battery
> slowly) rather than the 2A one (which will charge it with some
> cables).

Yes, being able to power device from external without charging the
battery is useful, too.

But I'd still like to control individual currents, too. If I have
power bank and two devices, I may want to select which one charges
faster.

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

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

[Pavel Machek]

Hi!

On Mon 2016-04-18 10:59:23, David Laight wrote:
> From: Pavel Machek
> > Sent: 18 April 2016 11:40
> ...
> > > >> > Actually, less is not stupid. Charging li-ion battery from li-ion battery might
> > > >> > be stupid. Imagine I'm on train, with device like N900 (50% battery) and power bank
> > > >> > (3Ah). I'm actively using the device. If I let it charge at full current, I'll waste
> > > >> > energy. If I limit current to approximately the power consumption, it will run the
> > > >> > powerbank empty, first, then empty the internal battery, maximizing total time I
> > > >> > can use the device.
> > > >>
> > > >> why would you waste energy ? What the charger chip would do is charge
> > > >> battery to maximum then just to maintenance charge from that point
> > > >> on. Where is energy being wasted other than normal heat dissipation ?
> > > >
> > > > Physics 101, of course wasted energy goes to heat. Lets not waste
> > > > energy by charging li-ion from li-ion when it is not required.
> > >
> > > your cellphone has no means to know that it's connected to a Li-Ion
> > > battery. We don't have visibility on what we're connected to, just how
> > > much it can source.
> > 
> > But cellphone user knows what he connected his charger to, and that's
> > why it is useful to be able to lower the current. Even when you said
> > "less is just stupid" I demonstrated it is not, at least in case when
> > your power source is a battery.
> 
> It reality you may want the phone/tablet to be configurable to take power
> from USB, but disable the li-ion charging circuit.
> That will maximise the time you get when running from an external battery.
> I connect my tablet to the 1A output (which discharges the internal battery
> slowly) rather than the 2A one (which will charge it with some
> cables).

Yes, being able to power device from external without charging the
battery is useful, too.

But I'd still like to control individual currents, too. If I have
power bank and two devices, I may want to select which one charges
faster.

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

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

[Baolin Wang]

On 31 March 2016 at 14:42, Felipe Balbi <balbi@kernel.org> wrote:
>>>> +#define DEFAULT_CUR_PROTECT  (50)
>>>
>>> Where is this coming from ? Also, () are not necessary.
>>
>> Just want to protect the default current limitation. If that does not
>> need, I'll remove it.
>
> It's your HW :-) You tell me if it's really necessary. But, hey, if you
> get enumerated @500mA, this is the host telling you it _CAN_ give you
> 500mA. In that case, why wouldn't you ?

Make sense. I'll remove the 'DEFAULT_CUR_PROTECT' macro.

>
>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>
>>> According to the spec we should always be talking about unit loads (1
>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>> actually). You're missing the opportunity of charging at 900mA.
>>
>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>> user can set them what they want.
>
> no, the user CANNOT set it to what they want. If you get enumerated
> @100mA and the user just decides to set it to 2000mA, s/he could even
> melt the USB connector. The kernel _must_ prevent such cases.
>
> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
> variable because if you enumerate in SS, you _can_ get up to 900mA.

Make sense. But these are just default values. They can be changed
safely by power driver with 'usb_charger_set_cur_limit_by_type()'
function to set 900mA.

>
>>>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>>>> +#define UCHGER_STATE_LENGTH  (50)
>>>
>>> what is this UCHGER_STATE_LENGTH ? And also, why don't you spell it out?
>>> Is this weird name coming from a spec ? Which spec ?
>>
>> It is used to indicate the array size to save the charger state to
>> report to userspace. I should move it to where it is used.
>
> and ARRAY_SIZE(arr) is not enough ?

OK.

>
>>> sure this fits as a bus_type. There's no "usb charger" bus. There's USB
>>> bus and its VBUS/GND lines. Why are you using a bus_type here ?
>>
>> I want to use bus structure to manage the charger device. Maybe choose
>> class to manage them?
>
> I guess a class would fit better in this case.

OK.

>
>>>> +{
>>>> +     return container_of(udev, struct usb_charger, dev);
>>>> +}
>>>> +
>>>> +static ssize_t sdp_limit_show(struct device *dev,
>>>> +                           struct device_attribute *attr,
>>>> +                           char *buf)
>>>> +{
>>>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>>>> +
>>>> +     return sprintf(buf, "%d\n", uchger->cur_limit.sdp_cur_limit);
>>>> +}
>>>> +
>>>> +static ssize_t sdp_limit_store(struct device *dev,
>>>> +                            struct device_attribute *attr,
>>>> +                            const char *buf, size_t count)
>>>> +{
>>>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>>>> +     unsigned int sdp_limit;
>>>> +     int ret;
>>>> +
>>>> +     ret = kstrtouint(buf, 10, &sdp_limit);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     ret = usb_charger_set_cur_limit_by_type(uchger, SDP_TYPE, sdp_limit);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     return count;
>>>> +}
>>>> +static DEVICE_ATTR_RW(sdp_limit);
>>>
>>> why RW ? Who's going to use these ? Also, you're not documenting this
>>> new sysfs file.
>>
>> Cause we have show and store operation for SDP type. If users want to
>> know or set the SDP current, they can use the sysfs file.
>> I'll add the documentation for it.
>
> but why would the user change it ? Here's the thing: you have a few
> posibilities for this:
>
> a) you are connected to a dedicated charger
>
>         In this case, you can get up to 2000mA depending on the charger.
>
>         If $this charger can give you or not 2000mA is not detectable,
>         so what do charging ICs do ? They slowly increase the attached
>         load accross VBUS/GND and measure VBUS value. When IC notices
>         VBUS dropping bit, step back to previous load.
>
>         This means you will always charger with maximum rating of DCP.
>
>         Why would user change this ? More is unsafe, less is just
>         stupid.
>
> b) you are connected to a host charging port and get enumerated with
> your 500mA configuration.
>
>         you *know* 500mA is okay, but you _can_ get more (it is a
>         charging port after all). So charging IC will connect a 500mA
>         load and step upwards until VBUS drops a little, just like (a)
>         above.
>
>         This means you will always charger with maximum rating for this
>         host charging port.
>
>         Why would user change this ? More is unsafe, less is just
>         stupid.
>
> c) you are connected to a standard port and get enumerated with your
> 500mA configuration.
>
>         you *know* 500mA is okay. So you connect a 500mA load and get it
>         over with.
>
>         This means you will always charger with maximum rating for this
>         SDP.
>
>         Why would user change this ? More is unsafe, less is just
>         stupid.
>
> d) you are connected to a standard port and get enumerated with your
> 100mA configuration.
>
>         you *know* 100mA is okay. So you connect a 100mA load and get it
>         over with.
>
>         This means you will always charger with maximum rating for this
>         SDP.
>
>         Why would user change this ? More is unsafe, less is just
>         stupid.
>
> do you see what I mean ? It's pointless to let this
> be-writeable. Whatever value user writes will either be unsafe or
> sub-optimal.

That sounds reasonable. Mark what do you think? Thanks.

>
> Just trust the charging IC to do a good job.
>
>>>> +
>>>> +/* USB charger state */
>>>> +enum usb_charger_state {
>>>> +     USB_CHARGER_DEFAULT,
>>>> +     USB_CHARGER_PRESENT,
>>>> +     USB_CHARGER_REMOVE,
>>>> +};
>>>
>>> userland really doesn't need these two ?
>>
>> We've reported to userspace by kobject_uevent in
>> 'usb_charger_notify_others()' function.
>
> I mean as a type ;-) So userspace doesn't have to redefine these for
> their applications.

Make sense. I can introduce some sysfs files for userspace. Thanks for
your comments.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Pavel Machek]

Hi!

> > It's your HW :-) You tell me if it's really necessary. But, hey, if you
> > get enumerated @500mA, this is the host telling you it _CAN_ give you
> > 500mA. In that case, why wouldn't you ?

Dunno, perhaps not to drain battery in host too quickly?
Or perhaps you are charging from external battery?

> >>> why RW ? Who's going to use these ? Also, you're not documenting this
> >>> new sysfs file.
> >>
> >> Cause we have show and store operation for SDP type. If users want to
> >> know or set the SDP current, they can use the sysfs file.
> >> I'll add the documentation for it.
> >
> > but why would the user change it ? Here's the thing: you have a few
> > posibilities for this:
> >
> > a) you are connected to a dedicated charger
> >
> >         In this case, you can get up to 2000mA depending on the charger.
> >
> >         If $this charger can give you or not 2000mA is not detectable,
> >         so what do charging ICs do ? They slowly increase the attached
> >         load accross VBUS/GND and measure VBUS value. When IC notices
> >         VBUS dropping bit, step back to previous load.
> >
> >         This means you will always charger with maximum rating of DCP.
> >
> >         Why would user change this ? More is unsafe, less is just
> >         stupid.

Less is not neccessarily stupid. First, it is useful for debugging, second, you
don't know how much this charger can give you. You measured you can get 1.8A,
but the note on the charger says 1.5A. You may want to go with 1.5A.

Also, there are several incompatible standards for detecting "dedicated charger". IIRC
iPhone has different one from iPad. So it is quite important to be able to control this manually.


> > d) you are connected to a standard port and get enumerated with your
> > 100mA configuration.
> >
> >         you *know* 100mA is okay. So you connect a 100mA load and get it
> >         over with.
> >
> >         This means you will always charger with maximum rating for this
> >         SDP.
> >
> >         Why would user change this ? More is unsafe, less is just
> >         stupid.

I've needed to override 100mA default many times. Maybe it is unsafe,
but it is useful.

(And with USB 5V connected directly to pretty beefy PC power supply...
it is sometimes safer than it looks).

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
> Hi!
>
>> > It's your HW :-) You tell me if it's really necessary. But, hey, if you
>> > get enumerated @500mA, this is the host telling you it _CAN_ give you
>> > 500mA. In that case, why wouldn't you ?
>
> Dunno, perhaps not to drain battery in host too quickly?
> Or perhaps you are charging from external battery?
>
>> >>> why RW ? Who's going to use these ? Also, you're not documenting this
>> >>> new sysfs file.
>> >>
>> >> Cause we have show and store operation for SDP type. If users want to
>> >> know or set the SDP current, they can use the sysfs file.
>> >> I'll add the documentation for it.
>> >
>> > but why would the user change it ? Here's the thing: you have a few
>> > posibilities for this:
>> >
>> > a) you are connected to a dedicated charger
>> >
>> >         In this case, you can get up to 2000mA depending on the charger.
>> >
>> >         If $this charger can give you or not 2000mA is not detectable,
>> >         so what do charging ICs do ? They slowly increase the attached
>> >         load accross VBUS/GND and measure VBUS value. When IC notices
>> >         VBUS dropping bit, step back to previous load.
>> >
>> >         This means you will always charger with maximum rating of DCP.
>> >
>> >         Why would user change this ? More is unsafe, less is just
>> >         stupid.
>
> Less is not neccessarily stupid. First, it is useful for debugging, second, you
> don't know how much this charger can give you. You measured you can get 1.8A,
> but the note on the charger says 1.5A. You may want to go with 1.5A.
>
> Also, there are several incompatible standards for detecting
> "dedicated charger". IIRC iPhone has different one from iPad. So it is
> quite important to be able to control this manually.

manually ??? Hell no! Charger IC should be able to do this no
problem. I would be surprised if there's any charger IC out there which
blindly connects a 1.8A load from the start. What these ICs do is that
they slowly increment the load and check voltage level. They'll continue
to do that up to the maximum you listed (1.8A, let's say). As soon as
voltage drops a bit, charger IC knows that it use previous load.

>> > d) you are connected to a standard port and get enumerated with your
>> > 100mA configuration.
>> >
>> >         you *know* 100mA is okay. So you connect a 100mA load and get it
>> >         over with.
>> >
>> >         This means you will always charger with maximum rating for this
>> >         SDP.
>> >
>> >         Why would user change this ? More is unsafe, less is just
>> >         stupid.
>
> I've needed to override 100mA default many times. Maybe it is unsafe,
> but it is useful.

still unsafe. If you really wanna do that, you're welcome to removing
safety margins from your own kernel, but we're definitely not going to
ship this to millions of users.

> (And with USB 5V connected directly to pretty beefy PC power supply...
> it is sometimes safer than it looks).

you're not considering the thermal dissipation on the USB connector
itself. Many of them might not use good metals because they assume the
maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
you could, literally, melt the connector.

-- 
balbi

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

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

[Pavel Machek]

Hi!

> >> > a) you are connected to a dedicated charger
> >> >
> >> >         In this case, you can get up to 2000mA depending on the charger.
> >> >
> >> >         If $this charger can give you or not 2000mA is not detectable,
> >> >         so what do charging ICs do ? They slowly increase the attached
> >> >         load accross VBUS/GND and measure VBUS value. When IC notices
> >> >         VBUS dropping bit, step back to previous load.
> >> >
> >> >         This means you will always charger with maximum rating of DCP.
> >> >
> >> >         Why would user change this ? More is unsafe, less is just
> >> >         stupid.
> >
> > Less is not neccessarily stupid. First, it is useful for debugging, second, you
> > don't know how much this charger can give you. You measured you can get 1.8A,
> > but the note on the charger says 1.5A. You may want to go with 1.5A.
> >
> > Also, there are several incompatible standards for detecting
> > "dedicated charger". IIRC iPhone has different one from iPad. So it is
> > quite important to be able to control this manually.
> 
> manually ??? Hell no! Charger IC should be able to do this no
> problem. I would be surprised if there's any charger IC out there which
> blindly connects a 1.8A load from the start. What these ICs do is that
> they slowly increment the load and check voltage level. They'll continue
> to do that up to the maximum you listed (1.8A, let's say). As soon as
> voltage drops a bit, charger IC knows that it use previous load.

As I explained, if the note on the wall charger says 1.5A, you want to
do 1.5A, not 1.8A. You can measure voltage on the charger, but you
don't know its temperature.

> >> > d) you are connected to a standard port and get enumerated with your
> >> > 100mA configuration.
> >> >
> >> >         you *know* 100mA is okay. So you connect a 100mA load and get it
> >> >         over with.
> >> >
> >> >         This means you will always charger with maximum rating for this
> >> >         SDP.
> >> >
> >> >         Why would user change this ? More is unsafe, less is just
> >> >         stupid.
> >
> > I've needed to override 100mA default many times. Maybe it is unsafe,
> > but it is useful.
> 
> still unsafe. If you really wanna do that, you're welcome to removing
> safety margins from your own kernel, but we're definitely not going to
> ship this to millions of users.

Not more unsafe than loading wall chargers with "lets see how much we
can get out of it" algorithm. Plus actually required to charge your
machines in useful way. So it is important that common API
exists. Whether it gets enalbed at production is different question.

Unfortunately, there's more than one standard for detecting charger,
so manual control will probably be required.

> > (And with USB 5V connected directly to pretty beefy PC power supply...
> > it is sometimes safer than it looks).
> 
> you're not considering the thermal dissipation on the USB connector
> itself. Many of them might not use good metals because they assume the
> maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
> you could, literally, melt the connector.

If you are dissipating 2.5W at the connector, you are doing something
very wrong. You should not be short-circuiting your USB... even when
the ports are usually designed to survive that.

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> >> > a) you are connected to a dedicated charger
>> >> >
>> >> >         In this case, you can get up to 2000mA depending on the charger.
>> >> >
>> >> >         If $this charger can give you or not 2000mA is not detectable,
>> >> >         so what do charging ICs do ? They slowly increase the attached
>> >> >         load accross VBUS/GND and measure VBUS value. When IC notices
>> >> >         VBUS dropping bit, step back to previous load.
>> >> >
>> >> >         This means you will always charger with maximum rating of DCP.
>> >> >
>> >> >         Why would user change this ? More is unsafe, less is just
>> >> >         stupid.
>> >
>> > Less is not neccessarily stupid. First, it is useful for debugging, second, you
>> > don't know how much this charger can give you. You measured you can get 1.8A,
>> > but the note on the charger says 1.5A. You may want to go with 1.5A.
>> >
>> > Also, there are several incompatible standards for detecting
>> > "dedicated charger". IIRC iPhone has different one from iPad. So it is
>> > quite important to be able to control this manually.
>> 
>> manually ??? Hell no! Charger IC should be able to do this no
>> problem. I would be surprised if there's any charger IC out there which
>> blindly connects a 1.8A load from the start. What these ICs do is that
>> they slowly increment the load and check voltage level. They'll continue
>> to do that up to the maximum you listed (1.8A, let's say). As soon as
>> voltage drops a bit, charger IC knows that it use previous load.
>
> As I explained, if the note on the wall charger says 1.5A, you want to
> do 1.5A, not 1.8A. You can measure voltage on the charger, but you
> don't know its temperature.

phone can't read what it says in the wall charger, nor can it know that
it's connected charger ABC and not charger XYZ. Think of the user
experience. You can't expect users to tell you "okay phone, the charger
reads that maximum is 1.5A, so please don't go over that."

>> >> > d) you are connected to a standard port and get enumerated with your
>> >> > 100mA configuration.
>> >> >
>> >> >         you *know* 100mA is okay. So you connect a 100mA load and get it
>> >> >         over with.
>> >> >
>> >> >         This means you will always charger with maximum rating for this
>> >> >         SDP.
>> >> >
>> >> >         Why would user change this ? More is unsafe, less is just
>> >> >         stupid.
>> >
>> > I've needed to override 100mA default many times. Maybe it is unsafe,
>> > but it is useful.
>> 
>> still unsafe. If you really wanna do that, you're welcome to removing
>> safety margins from your own kernel, but we're definitely not going to
>> ship this to millions of users.
>
> Not more unsafe than loading wall chargers with "lets see how much we
> can get out of it" algorithm. Plus actually required to charge your

it actually _is_ more unsafe. You could burn mother boards with that. If
host tells you it only has 100mA power budget left, why are you trying
to get more ?

> machines in useful way. So it is important that common API
> exists. Whether it gets enalbed at production is different question.

as I said, if you wanna do some unsafe manual method, be my guest; but
I'm not convinced that every user of the linux kernel wants that in
their pockets.

> Unfortunately, there's more than one standard for detecting charger,
> so manual control will probably be required.

n900 never had manual control of anything. It was just using information
given by the battery IC, charger IC and twl4030 madc.

>> > (And with USB 5V connected directly to pretty beefy PC power supply...
>> > it is sometimes safer than it looks).
>> 
>> you're not considering the thermal dissipation on the USB connector
>> itself. Many of them might not use good metals because they assume the
>> maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
>> you could, literally, melt the connector.
>
> If you are dissipating 2.5W at the connector, you are doing something
> very wrong. You should not be short-circuiting your USB... even when
> the ports are usually designed to survive that.

yes. You shouldn't. You also shouldn't go over that limit. If you have a
500mA total power budget, we should not let anybody try to draw more
because we have no control over what's on the side of the wire.

-- 
balbi

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

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

[Pavel Machek]

Hi!

> >> manually ??? Hell no! Charger IC should be able to do this no
> >> problem. I would be surprised if there's any charger IC out there which
> >> blindly connects a 1.8A load from the start. What these ICs do is that
> >> they slowly increment the load and check voltage level. They'll continue
> >> to do that up to the maximum you listed (1.8A, let's say). As soon as
> >> voltage drops a bit, charger IC knows that it use previous load.
> >
> > As I explained, if the note on the wall charger says 1.5A, you want to
> > do 1.5A, not 1.8A. You can measure voltage on the charger, but you
> > don't know its temperature.
> 
> phone can't read what it says in the wall charger, nor can it know that
> it's connected charger ABC and not charger XYZ. Think of the user
> experience. You can't expect users to tell you "okay phone, the charger
> reads that maximum is 1.5A, so please don't go over that."

Of course, we may do something sensible by default. But manual
controls should still be present. You called them "stupid" but they
are not.

Note that just because you detected wall charger does not even mean
you are connected to wall charger. See the link below.

> >> still unsafe. If you really wanna do that, you're welcome to removing
> >> safety margins from your own kernel, but we're definitely not going to
> >> ship this to millions of users.
> >
> > Not more unsafe than loading wall chargers with "lets see how much we
> > can get out of it" algorithm. Plus actually required to charge your
> 
> it actually _is_ more unsafe. You could burn mother boards with that. If
> host tells you it only has 100mA power budget left, why are you trying
> to get more ?

No, you can't burn motherboard like that... You can force emergency
shutdowns, which is also bad, but... There are many devices that break
this aspect of USB protocol.

https://www.kickstarter.com/projects/1785889318/doubbletime-charging-cable-full-battery-in-1-2-the

> > Unfortunately, there's more than one standard for detecting charger,
> > so manual control will probably be required.
> 
> n900 never had manual control of anything. It was just using information
> given by the battery IC, charger IC and twl4030 madc.

Manual control of n900 charging is done by:

echo 1800 > /sys/class/power_supply/bq24150a-0/current_limit

> >> > (And with USB 5V connected directly to pretty beefy PC power supply...
> >> > it is sometimes safer than it looks).
> >> 
> >> you're not considering the thermal dissipation on the USB connector
> >> itself. Many of them might not use good metals because they assume the
> >> maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
> >> you could, literally, melt the connector.
> >
> > If you are dissipating 2.5W at the connector, you are doing something
> > very wrong. You should not be short-circuiting your USB... even when
> > the ports are usually designed to survive that.
> 
> yes. You shouldn't. You also shouldn't go over that limit. If you have a
> 500mA total power budget, we should not let anybody try to draw more
> because we have no control over what's on the side of the wire.

They already can go over the limit, for example using cable linked
above. I have several such cables here. I also have various wall
supplies that are not detected as a wall supply by N900. So I either
have to remember and connect them with the "special" cable, or
(easier) use the override above to get useful charging.

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> >> manually ??? Hell no! Charger IC should be able to do this no
>> >> problem. I would be surprised if there's any charger IC out there which
>> >> blindly connects a 1.8A load from the start. What these ICs do is that
>> >> they slowly increment the load and check voltage level. They'll continue
>> >> to do that up to the maximum you listed (1.8A, let's say). As soon as
>> >> voltage drops a bit, charger IC knows that it use previous load.
>> >
>> > As I explained, if the note on the wall charger says 1.5A, you want to
>> > do 1.5A, not 1.8A. You can measure voltage on the charger, but you
>> > don't know its temperature.
>> 
>> phone can't read what it says in the wall charger, nor can it know that
>> it's connected charger ABC and not charger XYZ. Think of the user
>> experience. You can't expect users to tell you "okay phone, the charger
>> reads that maximum is 1.5A, so please don't go over that."
>
> Of course, we may do something sensible by default. But manual
> controls should still be present. You called them "stupid" but they
> are not.
>
> Note that just because you detected wall charger does not even mean
> you are connected to wall charger. See the link below.

that's a horrible product. So what ? If you want to use that, be my
guest. Just, again, don't ask for support when things start falling
apart ;-)

>> >> still unsafe. If you really wanna do that, you're welcome to removing
>> >> safety margins from your own kernel, but we're definitely not going to
>> >> ship this to millions of users.
>> >
>> > Not more unsafe than loading wall chargers with "lets see how much we
>> > can get out of it" algorithm. Plus actually required to charge your
>> 
>> it actually _is_ more unsafe. You could burn mother boards with that. If
>> host tells you it only has 100mA power budget left, why are you trying
>> to get more ?
>
> No, you can't burn motherboard like that... You can force emergency
> shutdowns, which is also bad, but... There are many devices that break
> this aspect of USB protocol.
>
> https://www.kickstarter.com/projects/1785889318/doubbletime-charging-cable-full-battery-in-1-2-the

we can't prevent people from coming up with bad devices/cables/whatever,
right ? But we can make sure that overcoming a 500mA power budget on a
USB 2.0 port will not be allowed.

>> > Unfortunately, there's more than one standard for detecting charger,
>> > so manual control will probably be required.
>> 
>> n900 never had manual control of anything. It was just using information
>> given by the battery IC, charger IC and twl4030 madc.
>
> Manual control of n900 charging is done by:
>
> echo 1800 > /sys/class/power_supply/bq24150a-0/current_limit

yes, that's fine. And if you're connected to a dedicated charger (DCP)
which follows USB Battery Charger specification, we *know* that it
should, per the spec, source up to 2A, so this is fine.

However, if you're connected to SDP (regular PC port), which has a power
budget of 500mA per-port (meaning, that if you're behind a bus powered
hub, you can't even get 500mA), then this write() of yours should be
invalid. It should *not* be a successful write() as that creates an
unsafe and potentially dangerous scenario for the user.

>> >> > (And with USB 5V connected directly to pretty beefy PC power supply...
>> >> > it is sometimes safer than it looks).
>> >> 
>> >> you're not considering the thermal dissipation on the USB connector
>> >> itself. Many of them might not use good metals because they assume the
>> >> maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
>> >> you could, literally, melt the connector.
>> >
>> > If you are dissipating 2.5W at the connector, you are doing something
>> > very wrong. You should not be short-circuiting your USB... even when
>> > the ports are usually designed to survive that.
>> 
>> yes. You shouldn't. You also shouldn't go over that limit. If you have a
>> 500mA total power budget, we should not let anybody try to draw more
>> because we have no control over what's on the side of the wire.
>
> They already can go over the limit, for example using cable linked
> above. I have several such cables here. I also have various wall

people can use unsupported cable assemblies if they want, but you can't
expect kernel to support you.

> supplies that are not detected as a wall supply by N900. So I either
> have to remember and connect them with the "special" cable, or
> (easier) use the override above to get useful charging.

those supplies are not supported by N900. N900 was designed with USB
Battery Chaging specification in mind and Nokia is not around anymore to
give you SW updates. Sorry, but that's not the kernel's fault.

The point is the following: there are a handful of people who would
*know* how to fiddle with these limits, many would not. The vast
majority would not. And, considering this is something completely out of
spec, and, again, potentially dangerous to the user, we are not going to
support it.

You may use your hacked up cables, not a problem. I did that myself
during N900 development (though I was using a lab power supply with a 2A
current limit sourcing 5V) to test port type detection and charging
algorithm. But that's really not something any company (or this
community) will support.

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> writes:
>> >> manually ??? Hell no! Charger IC should be able to do this no
>> >> problem. I would be surprised if there's any charger IC out there which
>> >> blindly connects a 1.8A load from the start. What these ICs do is that
>> >> they slowly increment the load and check voltage level. They'll continue
>> >> to do that up to the maximum you listed (1.8A, let's say). As soon as
>> >> voltage drops a bit, charger IC knows that it use previous load.
>> >
>> > As I explained, if the note on the wall charger says 1.5A, you want to
>> > do 1.5A, not 1.8A. You can measure voltage on the charger, but you
>> > don't know its temperature.
>> 
>> phone can't read what it says in the wall charger, nor can it know that
>> it's connected charger ABC and not charger XYZ. Think of the user
>> experience. You can't expect users to tell you "okay phone, the charger
>> reads that maximum is 1.5A, so please don't go over that."
>
> Of course, we may do something sensible by default. But manual
> controls should still be present. You called them "stupid" but they
> are not.
>
> Note that just because you detected wall charger does not even mean
> you are connected to wall charger. See the link below.

that's a horrible product. So what ? If you want to use that, be my
guest. Just, again, don't ask for support when things start falling
apart ;-)

>> >> still unsafe. If you really wanna do that, you're welcome to removing
>> >> safety margins from your own kernel, but we're definitely not going to
>> >> ship this to millions of users.
>> >
>> > Not more unsafe than loading wall chargers with "lets see how much we
>> > can get out of it" algorithm. Plus actually required to charge your
>> 
>> it actually _is_ more unsafe. You could burn mother boards with that. If
>> host tells you it only has 100mA power budget left, why are you trying
>> to get more ?
>
> No, you can't burn motherboard like that... You can force emergency
> shutdowns, which is also bad, but... There are many devices that break
> this aspect of USB protocol.
>
> https://www.kickstarter.com/projects/1785889318/doubbletime-charging-cable-full-battery-in-1-2-the

we can't prevent people from coming up with bad devices/cables/whatever,
right ? But we can make sure that overcoming a 500mA power budget on a
USB 2.0 port will not be allowed.

>> > Unfortunately, there's more than one standard for detecting charger,
>> > so manual control will probably be required.
>> 
>> n900 never had manual control of anything. It was just using information
>> given by the battery IC, charger IC and twl4030 madc.
>
> Manual control of n900 charging is done by:
>
> echo 1800 > /sys/class/power_supply/bq24150a-0/current_limit

yes, that's fine. And if you're connected to a dedicated charger (DCP)
which follows USB Battery Charger specification, we *know* that it
should, per the spec, source up to 2A, so this is fine.

However, if you're connected to SDP (regular PC port), which has a power
budget of 500mA per-port (meaning, that if you're behind a bus powered
hub, you can't even get 500mA), then this write() of yours should be
invalid. It should *not* be a successful write() as that creates an
unsafe and potentially dangerous scenario for the user.

>> >> > (And with USB 5V connected directly to pretty beefy PC power supply...
>> >> > it is sometimes safer than it looks).
>> >> 
>> >> you're not considering the thermal dissipation on the USB connector
>> >> itself. Many of them might not use good metals because they assume the
>> >> maximum power dissipated is 500mA * 5V = 2.5W. If you try to draw more,
>> >> you could, literally, melt the connector.
>> >
>> > If you are dissipating 2.5W at the connector, you are doing something
>> > very wrong. You should not be short-circuiting your USB... even when
>> > the ports are usually designed to survive that.
>> 
>> yes. You shouldn't. You also shouldn't go over that limit. If you have a
>> 500mA total power budget, we should not let anybody try to draw more
>> because we have no control over what's on the side of the wire.
>
> They already can go over the limit, for example using cable linked
> above. I have several such cables here. I also have various wall

people can use unsupported cable assemblies if they want, but you can't
expect kernel to support you.

> supplies that are not detected as a wall supply by N900. So I either
> have to remember and connect them with the "special" cable, or
> (easier) use the override above to get useful charging.

those supplies are not supported by N900. N900 was designed with USB
Battery Chaging specification in mind and Nokia is not around anymore to
give you SW updates. Sorry, but that's not the kernel's fault.

The point is the following: there are a handful of people who would
*know* how to fiddle with these limits, many would not. The vast
majority would not. And, considering this is something completely out of
spec, and, again, potentially dangerous to the user, we are not going to
support it.

You may use your hacked up cables, not a problem. I did that myself
during N900 development (though I was using a lab power supply with a 2A
current limit sourcing 5V) to test port type detection and charging
algorithm. But that's really not something any company (or this
community) will support.

-- 
balbi

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

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

[Pavel Machek]

Hi!

> > Of course, we may do something sensible by default. But manual
> > controls should still be present. You called them "stupid" but they
> > are not.
> >
> > Note that just because you detected wall charger does not even mean
> > you are connected to wall charger. See the link below.
> 
> that's a horrible product. So what ? If you want to use that, be my
> guest. Just, again, don't ask for support when things start falling
> apart ;-)

So you have USB spec? So what? There are many such products out there,
and I have at least two such cables here.

They did not cause end of the world, yet, and they are actually very useful.

> >> >> still unsafe. If you really wanna do that, you're welcome to removing
> >> >> safety margins from your own kernel, but we're definitely not going to
> >> >> ship this to millions of users.
> >> >
> >> > Not more unsafe than loading wall chargers with "lets see how much we
> >> > can get out of it" algorithm. Plus actually required to charge your
> >> 
> >> it actually _is_ more unsafe. You could burn mother boards with that. If
> >> host tells you it only has 100mA power budget left, why are you trying
> >> to get more ?
> >
> > No, you can't burn motherboard like that... You can force emergency
> > shutdowns, which is also bad, but... There are many devices that break
> > this aspect of USB protocol.
> >
> > https://www.kickstarter.com/projects/1785889318/doubbletime-charging-cable-full-battery-in-1-2-the
> 
> we can't prevent people from coming up with bad devices/cables/whatever,
> right ? But we can make sure that overcoming a 500mA power budget on a
> USB 2.0 port will not be allowed.

No, you can't, because you don't know if you are connected to USB 2.0
port. (Because non-compliant cables exist).

> >> > Unfortunately, there's more than one standard for detecting charger,
> >> > so manual control will probably be required.
> >> 
> >> n900 never had manual control of anything. It was just using information
> >> given by the battery IC, charger IC and twl4030 madc.
> >
> > Manual control of n900 charging is done by:
> >
> > echo 1800 > /sys/class/power_supply/bq24150a-0/current_limit
> 
> yes, that's fine. And if you're connected to a dedicated charger (DCP)
> which follows USB Battery Charger specification, we *know* that it
> should, per the spec, source up to 2A, so this is fine.

BTW have you ever seen such USB-compliant dedicated charger? I have
more than 5 chargers here, and not one of them is 2A. (Most are .5A,
some are 1A, one is 1.2A).

> However, if you're connected to SDP (regular PC port), which has a power
> budget of 500mA per-port (meaning, that if you're behind a bus powered
> hub, you can't even get 500mA), then this write() of yours should be
> invalid. It should *not* be a successful write() as that creates an
> unsafe and potentially dangerous scenario for the user.

Yes, USB is "potentially dangerous", because noone follows the
specs. Fortunately, everyone knows (except you?) that noone follows
the specs, so the hardware can deal with that, and they include
(poly?) fuses where neccessary.

> > They already can go over the limit, for example using cable linked
> > above. I have several such cables here. I also have various wall
> 
> people can use unsupported cable assemblies if they want, but you can't
> expect kernel to support you.

Then we won't have useful charging support in kernel.

> > supplies that are not detected as a wall supply by N900. So I either
> > have to remember and connect them with the "special" cable, or
> > (easier) use the override above to get useful charging.
> 
> those supplies are not supported by N900. N900 was designed with USB
> Battery Chaging specification in mind and Nokia is not around anymore to
> give you SW updates. Sorry, but that's not the kernel's fault.
> 
> The point is the following: there are a handful of people who would
> *know* how to fiddle with these limits, many would not. The vast
> majority would not. And, considering this is something completely out of
> spec, and, again, potentially dangerous to the user, we are not going to
> support it.

You speak about dangerous where little danger exist; number of
non-compliant cables and USB devices is very high (take your power
bank. Does it really limit to .5A when charging from computer?) and we
should support them, not cry "dangerous" and force everyone to come
with their own "solutions".

> You may use your hacked up cables, not a problem. I did that myself
> during N900 development (though I was using a lab power supply with a 2A
> current limit sourcing 5V) to test port type detection and charging
> algorithm. But that's really not something any company (or this
> community) will support.

Fortunately, that's not your decision and community already decided
the other way.
									Pavel

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

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

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
>> > Of course, we may do something sensible by default. But manual
>> > controls should still be present. You called them "stupid" but they
>> > are not.
>> >
>> > Note that just because you detected wall charger does not even mean
>> > you are connected to wall charger. See the link below.
>> 
>> that's a horrible product. So what ? If you want to use that, be my
>> guest. Just, again, don't ask for support when things start falling
>> apart ;-)
>
> So you have USB spec? So what? There are many such products out there,
> and I have at least two such cables here.
>
> They did not cause end of the world, yet, and they are actually very
> useful.

they are also breaking safety requirements and, as I already said, are
potentially dangerous to the user.

>> >> >> still unsafe. If you really wanna do that, you're welcome to removing
>> >> >> safety margins from your own kernel, but we're definitely not going to
>> >> >> ship this to millions of users.
>> >> >
>> >> > Not more unsafe than loading wall chargers with "lets see how much we
>> >> > can get out of it" algorithm. Plus actually required to charge your
>> >> 
>> >> it actually _is_ more unsafe. You could burn mother boards with that. If
>> >> host tells you it only has 100mA power budget left, why are you trying
>> >> to get more ?
>> >
>> > No, you can't burn motherboard like that... You can force emergency
>> > shutdowns, which is also bad, but... There are many devices that break
>> > this aspect of USB protocol.
>> >
>> > https://www.kickstarter.com/projects/1785889318/doubbletime-charging-cable-full-battery-in-1-2-the
>> 
>> we can't prevent people from coming up with bad devices/cables/whatever,
>> right ? But we can make sure that overcoming a 500mA power budget on a
>> USB 2.0 port will not be allowed.
>
> No, you can't, because you don't know if you are connected to USB 2.0
> port. (Because non-compliant cables exist).

yeah, we can't also protect car passenger from bad drivers, but we can
give them all a seat belt ;-)

>> >> > Unfortunately, there's more than one standard for detecting charger,
>> >> > so manual control will probably be required.
>> >> 
>> >> n900 never had manual control of anything. It was just using information
>> >> given by the battery IC, charger IC and twl4030 madc.
>> >
>> > Manual control of n900 charging is done by:
>> >
>> > echo 1800 > /sys/class/power_supply/bq24150a-0/current_limit
>> 
>> yes, that's fine. And if you're connected to a dedicated charger (DCP)
>> which follows USB Battery Charger specification, we *know* that it
>> should, per the spec, source up to 2A, so this is fine.
>
> BTW have you ever seen such USB-compliant dedicated charger? I have
> more than 5 chargers here, and not one of them is 2A. (Most are .5A,
> some are 1A, one is 1.2A).

yeah, if it has the USB BC logo, it _must_ be compliant and full all
requirements of the USB BC spec.

>> However, if you're connected to SDP (regular PC port), which has a power
>> budget of 500mA per-port (meaning, that if you're behind a bus powered
>> hub, you can't even get 500mA), then this write() of yours should be
>> invalid. It should *not* be a successful write() as that creates an
>> unsafe and potentially dangerous scenario for the user.
>
> Yes, USB is "potentially dangerous", because noone follows the
> specs. Fortunately, everyone knows (except you?) that noone follows
> the specs, so the hardware can deal with that, and they include
> (poly?) fuses where neccessary.

heh :-)

>> > They already can go over the limit, for example using cable linked
>> > above. I have several such cables here. I also have various wall
>> 
>> people can use unsupported cable assemblies if they want, but you can't
>> expect kernel to support you.
>
> Then we won't have useful charging support in kernel.

oh no, we will. We just won't let users step outside of safety requirements.

>> > supplies that are not detected as a wall supply by N900. So I either
>> > have to remember and connect them with the "special" cable, or
>> > (easier) use the override above to get useful charging.
>> 
>> those supplies are not supported by N900. N900 was designed with USB
>> Battery Chaging specification in mind and Nokia is not around anymore to
>> give you SW updates. Sorry, but that's not the kernel's fault.
>> 
>> The point is the following: there are a handful of people who would
>> *know* how to fiddle with these limits, many would not. The vast
>> majority would not. And, considering this is something completely out of
>> spec, and, again, potentially dangerous to the user, we are not going to
>> support it.
>
> You speak about dangerous where little danger exist; number of
> non-compliant cables and USB devices is very high (take your power
> bank. Does it really limit to .5A when charging from computer?) and we
> should support them, not cry "dangerous" and force everyone to come
> with their own "solutions".

hehe, you're pretty funny at times

>> You may use your hacked up cables, not a problem. I did that myself
>> during N900 development (though I was using a lab power supply with a 2A
>> current limit sourcing 5V) to test port type detection and charging
>> algorithm. But that's really not something any company (or this
>> community) will support.
>
> Fortunately, that's not your decision and community already decided
> the other way.

actually, it has not ;-) As long as it's not in Linus' tree, it has not
been decided yet.

now, chill out dude. You're always pushy and arrogant instead of
discussing things technically. Calm down, yeah !?

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>>
>>>> According to the spec we should always be talking about unit loads (1
>>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>>> actually). You're missing the opportunity of charging at 900mA.
>>>
>>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>>> user can set them what they want.
>>
>> no, the user CANNOT set it to what they want. If you get enumerated
>> @100mA and the user just decides to set it to 2000mA, s/he could even
>> melt the USB connector. The kernel _must_ prevent such cases.
>>
>> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
>> variable because if you enumerate in SS, you _can_ get up to 900mA.
>
> Make sense. But these are just default values. They can be changed
> safely by power driver with 'usb_charger_set_cur_limit_by_type()'
> function to set 900mA.

oh okay. Still, the default value should be a function of gadget->speed,
IMO ;-)

>>>>> +
>>>>> +/* USB charger state */
>>>>> +enum usb_charger_state {
>>>>> +     USB_CHARGER_DEFAULT,
>>>>> +     USB_CHARGER_PRESENT,
>>>>> +     USB_CHARGER_REMOVE,
>>>>> +};
>>>>
>>>> userland really doesn't need these two ?
>>>
>>> We've reported to userspace by kobject_uevent in
>>> 'usb_charger_notify_others()' function.
>>
>> I mean as a type ;-) So userspace doesn't have to redefine these for
>> their applications.
>
> Make sense. I can introduce some sysfs files for userspace. Thanks for
> your comments.

okay, my reply was a bit cryptic, but what I mean here is that enum
usb_charger_state could be moved your include/uapi header. My question
is, then, does userland need to have knowledge of enum
usb_charger_state ?

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>>
>>>> According to the spec we should always be talking about unit loads (1
>>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>>> actually). You're missing the opportunity of charging at 900mA.
>>>
>>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>>> user can set them what they want.
>>
>> no, the user CANNOT set it to what they want. If you get enumerated
>> @100mA and the user just decides to set it to 2000mA, s/he could even
>> melt the USB connector. The kernel _must_ prevent such cases.
>>
>> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
>> variable because if you enumerate in SS, you _can_ get up to 900mA.
>
> Make sense. But these are just default values. They can be changed
> safely by power driver with 'usb_charger_set_cur_limit_by_type()'
> function to set 900mA.

oh okay. Still, the default value should be a function of gadget->speed,
IMO ;-)

>>>>> +
>>>>> +/* USB charger state */
>>>>> +enum usb_charger_state {
>>>>> +     USB_CHARGER_DEFAULT,
>>>>> +     USB_CHARGER_PRESENT,
>>>>> +     USB_CHARGER_REMOVE,
>>>>> +};
>>>>
>>>> userland really doesn't need these two ?
>>>
>>> We've reported to userspace by kobject_uevent in
>>> 'usb_charger_notify_others()' function.
>>
>> I mean as a type ;-) So userspace doesn't have to redefine these for
>> their applications.
>
> Make sense. I can introduce some sysfs files for userspace. Thanks for
> your comments.

okay, my reply was a bit cryptic, but what I mean here is that enum
usb_charger_state could be moved your include/uapi header. My question
is, then, does userland need to have knowledge of enum
usb_charger_state ?

-- 
balbi

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

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

[Baolin Wang]

On 31 March 2016 at 16:18, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>>>
>>>>> According to the spec we should always be talking about unit loads (1
>>>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>>>> actually). You're missing the opportunity of charging at 900mA.
>>>>
>>>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>>>> user can set them what they want.
>>>
>>> no, the user CANNOT set it to what they want. If you get enumerated
>>> @100mA and the user just decides to set it to 2000mA, s/he could even
>>> melt the USB connector. The kernel _must_ prevent such cases.
>>>
>>> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
>>> variable because if you enumerate in SS, you _can_ get up to 900mA.
>>
>> Make sense. But these are just default values. They can be changed
>> safely by power driver with 'usb_charger_set_cur_limit_by_type()'
>> function to set 900mA.
>
> oh okay. Still, the default value should be a function of gadget->speed,

Sorry, I did not get your suggestion, could you give me an example? Thanks.

> IMO ;-)
>
>>>>>> +
>>>>>> +/* USB charger state */
>>>>>> +enum usb_charger_state {
>>>>>> +     USB_CHARGER_DEFAULT,
>>>>>> +     USB_CHARGER_PRESENT,
>>>>>> +     USB_CHARGER_REMOVE,
>>>>>> +};
>>>>>
>>>>> userland really doesn't need these two ?
>>>>
>>>> We've reported to userspace by kobject_uevent in
>>>> 'usb_charger_notify_others()' function.
>>>
>>> I mean as a type ;-) So userspace doesn't have to redefine these for
>>> their applications.
>>
>> Make sense. I can introduce some sysfs files for userspace. Thanks for
>> your comments.
>
> okay, my reply was a bit cryptic, but what I mean here is that enum
> usb_charger_state could be moved your include/uapi header. My question
> is, then, does userland need to have knowledge of enum
> usb_charger_state ?

I am not sure if userland need the enum usb_charger_state. But I
remember you want to report the charger state to userland in previous
email.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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

Baolin Wang <baolin.wang@linaro.org> writes:
> [ text/plain ]
> On 31 March 2016 at 16:18, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>>>>
>>>>>> According to the spec we should always be talking about unit loads (1
>>>>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>>>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>>>>> actually). You're missing the opportunity of charging at 900mA.
>>>>>
>>>>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>>>>> user can set them what they want.
>>>>
>>>> no, the user CANNOT set it to what they want. If you get enumerated
>>>> @100mA and the user just decides to set it to 2000mA, s/he could even
>>>> melt the USB connector. The kernel _must_ prevent such cases.
>>>>
>>>> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
>>>> variable because if you enumerate in SS, you _can_ get up to 900mA.
>>>
>>> Make sense. But these are just default values. They can be changed
>>> safely by power driver with 'usb_charger_set_cur_limit_by_type()'
>>> function to set 900mA.
>>
>> oh okay. Still, the default value should be a function of gadget->speed,
>
> Sorry, I did not get your suggestion, could you give me an example? Thanks.

int default_current_limit = 500;

if (gadget->speed >= USB_SPEED_SUPER)
	default_current_limit = 900;

>>>>>>> +
>>>>>>> +/* USB charger state */
>>>>>>> +enum usb_charger_state {
>>>>>>> +     USB_CHARGER_DEFAULT,
>>>>>>> +     USB_CHARGER_PRESENT,
>>>>>>> +     USB_CHARGER_REMOVE,
>>>>>>> +};
>>>>>>
>>>>>> userland really doesn't need these two ?
>>>>>
>>>>> We've reported to userspace by kobject_uevent in
>>>>> 'usb_charger_notify_others()' function.
>>>>
>>>> I mean as a type ;-) So userspace doesn't have to redefine these for
>>>> their applications.
>>>
>>> Make sense. I can introduce some sysfs files for userspace. Thanks for
>>> your comments.
>>
>> okay, my reply was a bit cryptic, but what I mean here is that enum
>> usb_charger_state could be moved your include/uapi header. My question
>> is, then, does userland need to have knowledge of enum
>> usb_charger_state ?
>
> I am not sure if userland need the enum usb_charger_state. But I
> remember you want to report the charger state to userland in previous
> email.

right, which means this enumeration definition could be placed in the
UAPI header. Unless, of course, we're reporting strings, rather than
integers, in the sysfs file.

-- 
balbi

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

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

[Baolin Wang]

On 31 March 2016 at 18:06, Felipe Balbi <balbi@kernel.org> wrote:
> Baolin Wang <baolin.wang@linaro.org> writes:
>> [ text/plain ]
>> On 31 March 2016 at 16:18, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>>>>>>>
>>>>>>> According to the spec we should always be talking about unit loads (1
>>>>>>> unit load is 100mA for HS/FS/LS and 150mA for SS). Also, this will not
>>>>>>> work for SS capable ports and SS gadgets (we have quite a few of them,
>>>>>>> actually). You're missing the opportunity of charging at 900mA.
>>>>>>
>>>>>> I follow the DCP/SDP/CDP/ACA type's default current limitation and
>>>>>> user can set them what they want.
>>>>>
>>>>> no, the user CANNOT set it to what they want. If you get enumerated
>>>>> @100mA and the user just decides to set it to 2000mA, s/he could even
>>>>> melt the USB connector. The kernel _must_ prevent such cases.
>>>>>
>>>>> In any case, DEFAULT_SDP_CUR_LIMIT shouldn't be a constant, it should be
>>>>> variable because if you enumerate in SS, you _can_ get up to 900mA.
>>>>
>>>> Make sense. But these are just default values. They can be changed
>>>> safely by power driver with 'usb_charger_set_cur_limit_by_type()'
>>>> function to set 900mA.
>>>
>>> oh okay. Still, the default value should be a function of gadget->speed,
>>
>> Sorry, I did not get your suggestion, could you give me an example? Thanks.
>
> int default_current_limit = 500;
>
> if (gadget->speed >= USB_SPEED_SUPER)
>         default_current_limit = 900;

Make sense.

>
>>>>>>>> +
>>>>>>>> +/* USB charger state */
>>>>>>>> +enum usb_charger_state {
>>>>>>>> +     USB_CHARGER_DEFAULT,
>>>>>>>> +     USB_CHARGER_PRESENT,
>>>>>>>> +     USB_CHARGER_REMOVE,
>>>>>>>> +};
>>>>>>>
>>>>>>> userland really doesn't need these two ?
>>>>>>
>>>>>> We've reported to userspace by kobject_uevent in
>>>>>> 'usb_charger_notify_others()' function.
>>>>>
>>>>> I mean as a type ;-) So userspace doesn't have to redefine these for
>>>>> their applications.
>>>>
>>>> Make sense. I can introduce some sysfs files for userspace. Thanks for
>>>> your comments.
>>>
>>> okay, my reply was a bit cryptic, but what I mean here is that enum
>>> usb_charger_state could be moved your include/uapi header. My question
>>> is, then, does userland need to have knowledge of enum
>>> usb_charger_state ?
>>
>> I am not sure if userland need the enum usb_charger_state. But I
>> remember you want to report the charger state to userland in previous
>> email.
>
> right, which means this enumeration definition could be placed in the
> UAPI header. Unless, of course, we're reporting strings, rather than
> integers, in the sysfs file.

OK. Thanks.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Mark Brown]

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

On Thu, Mar 31, 2016 at 09:42:58AM +0300, Felipe Balbi wrote:
> Baolin Wang <baolin.wang@linaro.org> writes:

> > I want to use bus structure to manage the charger device. Maybe choose
> > class to manage them?

> I guess a class would fit better in this case.

IIRC Greg didn't want new classes?

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

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

[Felipe Balbi]

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


Hi,

Mark Brown <broonie@kernel.org> writes:
> On Thu, Mar 31, 2016 at 09:42:58AM +0300, Felipe Balbi wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>
>> > I want to use bus structure to manage the charger device. Maybe choose
>> > class to manage them?
>
>> I guess a class would fit better in this case.
>
> IIRC Greg didn't want new classes?

good point. Still, this doesn't seem to fit a but_type IMO.

-- 
balbi

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

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

[Mark Brown]

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

On Fri, Apr 01, 2016 at 08:43:10AM +0300, Felipe Balbi wrote:
> Mark Brown <broonie@kernel.org> writes:

> > IIRC Greg didn't want new classes?

> good point. Still, this doesn't seem to fit a but_type IMO.

It does in this new world order.  IIRC on an earlier round of review
there was some code that didn't use a bus but that got complaints that
it was trying to reimplement the bus functionality.

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

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

[Felipe Balbi]

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


Hi,

Mark Brown <broonie@kernel.org> writes:
> On Fri, Apr 01, 2016 at 08:43:10AM +0300, Felipe Balbi wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> > IIRC Greg didn't want new classes?
>
>> good point. Still, this doesn't seem to fit a but_type IMO.
>
> It does in this new world order.  IIRC on an earlier round of review
> there was some code that didn't use a bus but that got complaints that
> it was trying to reimplement the bus functionality.

fair enough, I'll wait for Greg to have some time to comment on
this. Bottomline is that there is *no* real bus. Charger ICs will use
SPI or I2C and that's a real bus, $subject is not.

-- 
balbi

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

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

[Mark Brown]

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

On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote:
> Mark Brown <broonie@kernel.org> writes:

> > It does in this new world order.  IIRC on an earlier round of review
> > there was some code that didn't use a bus but that got complaints that
> > it was trying to reimplement the bus functionality.

> fair enough, I'll wait for Greg to have some time to comment on
> this. Bottomline is that there is *no* real bus. Charger ICs will use

I've got a feeling Greg is zoning this out by now...

> SPI or I2C and that's a real bus, $subject is not.

Well, there's the physical connection between the system power supply
and the USB port if you're very keen on looking for hardware.

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

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

[Mark Brown]

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

On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> > It does in this new world order.  IIRC on an earlier round of review
> > there was some code that didn't use a bus but that got complaints that
> > it was trying to reimplement the bus functionality.

> fair enough, I'll wait for Greg to have some time to comment on
> this. Bottomline is that there is *no* real bus. Charger ICs will use

I've got a feeling Greg is zoning this out by now...

> SPI or I2C and that's a real bus, $subject is not.

Well, there's the physical connection between the system power supply
and the USB port if you're very keen on looking for hardware.

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

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

[Greg KH]

On Mon, Apr 04, 2016 at 09:04:48AM -0700, Mark Brown wrote:
> On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote:
> > Mark Brown <broonie@kernel.org> writes:
> 
> > > It does in this new world order.  IIRC on an earlier round of review
> > > there was some code that didn't use a bus but that got complaints that
> > > it was trying to reimplement the bus functionality.
> 
> > fair enough, I'll wait for Greg to have some time to comment on
> > this. Bottomline is that there is *no* real bus. Charger ICs will use
> 
> I've got a feeling Greg is zoning this out by now...

Very true :)

I have a lot of other patches to go through this week, it will be a
while before I get to these...

thanks,

greg k-h

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

[Baolin Wang]

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

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

Introduce a callback 'get_charger_type' which will implemented by
user for usb gadget operations to get the usb charger type.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index fd73a3e..1ac2b40 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/usb_charger.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -230,6 +231,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");
 }
@@ -422,8 +426,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	mutex_unlock(&udc_lock);
 
+	ret = usb_charger_init(gadget);
+	if (ret)
+		goto err5;
+
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -502,6 +512,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 d82d006..024b33d 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/usb_charger.h>
 
 struct usb_ep;
 
@@ -563,6 +564,7 @@ struct usb_gadget_ops {
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
+	enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -635,6 +637,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	/* negotiate the power with the usb charger */
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -839,10 +843,17 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+	if (gadget->charger)
+		usb_charger_set_cur_limit_by_type(gadget->charger,
+				usb_charger_detect_type(gadget->charger), mA);
+
 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

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

[kbuild test robot]

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

Hi Baolin,

[auto build test WARNING on v4.5-rc7]
[also build test WARNING on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20160316-195102
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/usb/gadget.h:228: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:228: warning: No description found for parameter 'enabled'
>> include/linux/usb/gadget.h:656: warning: No description found for parameter 'charger'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_zlp_not_supp'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef member 'setup_pending' description in 'usb_composite_dev'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef member 'os_desc_pending' description in 'usb_composite_dev'
   drivers/usb/gadget/function/f_acm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_ecm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_subset.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_obex.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_serial.c:1: warning: no structured comments found

vim +/charger +656 include/linux/usb/gadget.h

77e9162f include/linux/usb/gadget.h Baolin Wang     2016-03-16  640  	/* negotiate the power with the usb charger */
77e9162f include/linux/usb/gadget.h Baolin Wang     2016-03-16  641  	struct usb_charger		*charger;
d8318d7f include/linux/usb/gadget.h David Cohen     2013-12-09  642  
898c6086 include/linux/usb/gadget.h Felipe Balbi    2011-11-22  643  	unsigned			sg_supported:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  644  	unsigned			is_otg:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  645  	unsigned			is_a_peripheral:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  646  	unsigned			b_hnp_enable:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  647  	unsigned			a_hnp_support:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  648  	unsigned			a_alt_hnp_support:1;
0b2d2bba include/linux/usb/gadget.h David Cohen     2013-12-09  649  	unsigned			quirk_ep_out_aligned_size:1;
ffd9a0fc include/linux/usb/gadget.h Robert Baldyga  2015-07-28  650  	unsigned			quirk_altset_not_supp:1;
02ded1b0 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  651  	unsigned			quirk_stall_not_supp:1;
ca1023c8 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  652  	unsigned			quirk_zlp_not_supp:1;
80b2502c include/linux/usb/gadget.h Peter Chen      2015-01-28  653  	unsigned			is_selfpowered:1;
ccdf138f include/linux/usb/gadget.h Robert Baldyga  2015-05-04  654  	unsigned			deactivated:1;
ccdf138f include/linux/usb/gadget.h Robert Baldyga  2015-05-04  655  	unsigned			connected:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16 @656  };
5702f753 include/linux/usb/gadget.h Felipe Balbi    2013-07-17  657  #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  658  
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  659  static inline void set_gadget_data(struct usb_gadget *gadget, void *data)
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  660  	{ dev_set_drvdata(&gadget->dev, data); }
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  661  static inline void *get_gadget_data(struct usb_gadget *gadget)
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  662  	{ return dev_get_drvdata(&gadget->dev); }
f48cf80f include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  663  static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
f48cf80f include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  664  {

:::::: The code at line 656 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6229 bytes --]

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

[kbuild test robot]

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

Hi Baolin,

[auto build test WARNING on v4.5-rc7]
[also build test WARNING on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20160316-195102
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/usb/gadget.h:228: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:228: warning: No description found for parameter 'enabled'
>> include/linux/usb/gadget.h:656: warning: No description found for parameter 'charger'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:656: warning: No description found for parameter 'quirk_zlp_not_supp'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef member 'setup_pending' description in 'usb_composite_dev'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef member 'os_desc_pending' description in 'usb_composite_dev'
   drivers/usb/gadget/function/f_acm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_ecm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_subset.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_obex.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_serial.c:1: warning: no structured comments found

vim +/charger +656 include/linux/usb/gadget.h

77e9162f include/linux/usb/gadget.h Baolin Wang     2016-03-16  640  	/* negotiate the power with the usb charger */
77e9162f include/linux/usb/gadget.h Baolin Wang     2016-03-16  641  	struct usb_charger		*charger;
d8318d7f include/linux/usb/gadget.h David Cohen     2013-12-09  642  
898c6086 include/linux/usb/gadget.h Felipe Balbi    2011-11-22  643  	unsigned			sg_supported:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  644  	unsigned			is_otg:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  645  	unsigned			is_a_peripheral:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  646  	unsigned			b_hnp_enable:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  647  	unsigned			a_hnp_support:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  648  	unsigned			a_alt_hnp_support:1;
0b2d2bba include/linux/usb/gadget.h David Cohen     2013-12-09  649  	unsigned			quirk_ep_out_aligned_size:1;
ffd9a0fc include/linux/usb/gadget.h Robert Baldyga  2015-07-28  650  	unsigned			quirk_altset_not_supp:1;
02ded1b0 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  651  	unsigned			quirk_stall_not_supp:1;
ca1023c8 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  652  	unsigned			quirk_zlp_not_supp:1;
80b2502c include/linux/usb/gadget.h Peter Chen      2015-01-28  653  	unsigned			is_selfpowered:1;
ccdf138f include/linux/usb/gadget.h Robert Baldyga  2015-05-04  654  	unsigned			deactivated:1;
ccdf138f include/linux/usb/gadget.h Robert Baldyga  2015-05-04  655  	unsigned			connected:1;
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16 @656  };
5702f753 include/linux/usb/gadget.h Felipe Balbi    2013-07-17  657  #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  658  
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  659  static inline void set_gadget_data(struct usb_gadget *gadget, void *data)
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  660  	{ dev_set_drvdata(&gadget->dev, data); }
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  661  static inline void *get_gadget_data(struct usb_gadget *gadget)
^1da177e include/linux/usb_gadget.h Linus Torvalds  2005-04-16  662  	{ return dev_get_drvdata(&gadget->dev); }
f48cf80f include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  663  static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
f48cf80f include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  664  {

:::::: The code at line 656 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6229 bytes --]

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

[kbuild test robot]

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

Hi Baolin,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20160316-195102
config: x86_64-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "usb_charger_init" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_plug_by_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_exit" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 53130 bytes --]

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

[kbuild test robot]

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

Hi Baolin,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20160316-195102
config: x86_64-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "usb_charger_init" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_plug_by_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_exit" [drivers/usb/gadget/udc/udc-core.ko] undefined!
>> ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 53130 bytes --]

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

[Baolin Wang]

When the usb gadget supporting for usb charger is ready, the usb charger
should get the type by the 'get_charger_type' callback which is implemented
by the usb gadget operations, and get the usb charger pointer from struct
'usb_gadget'.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/gadget/charger.c |   43 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
index 82a9973..76e1a6f 100644
--- a/drivers/usb/gadget/charger.c
+++ b/drivers/usb/gadget/charger.c
@@ -272,7 +272,11 @@ EXPORT_SYMBOL_GPL(usb_charger_unregister_notify);
 enum usb_charger_type
 usb_charger_detect_type(struct usb_charger *uchger)
 {
-	if (uchger->psy) {
+	if (uchger->gadget && uchger->gadget->ops
+	    && uchger->gadget->ops->get_charger_type) {
+		uchger->type =
+			uchger->gadget->ops->get_charger_type(uchger->gadget);
+	} else if (uchger->psy) {
 		union power_supply_propval val;
 
 		power_supply_get_property(uchger->psy,
@@ -479,6 +483,29 @@ 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 (!uchger)
+		return -EINVAL;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger state is changed
+	 * with detecting by usb gadget state.
+	 */
+	if (uchger->old_gadget_state != state) {
+		uchger->old_gadget_state = state;
+
+		if (state >= USB_STATE_ATTACHED)
+			uchger_state = USB_CHARGER_PRESENT;
+		else if (state == USB_STATE_NOTATTACHED)
+			uchger_state = USB_CHARGER_REMOVE;
+		else
+			uchger_state = USB_CHARGER_DEFAULT;
+
+		usb_charger_notify_others(uchger, uchger_state);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
@@ -635,6 +662,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 = ugadget->state;
 
 	/* register a new usb charger */
@@ -655,7 +683,18 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-	return 0;
+	struct usb_charger *uchger = ugadget->charger;
+
+	if (!uchger)
+		return -EINVAL;
+
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+
+	return usb_charger_unregister(uchger);
 }
 
 static int __init usb_charger_sysfs_init(void)
-- 
1.7.9.5

[PATCH v7 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..043f1f4 100644
--- a/drivers/power/wm831x_power.c
+++ b/drivers/power/wm831x_power.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
+#include <linux/usb/usb_charger.h>
 
 #include <linux/mfd/wm831x/core.h>
 #include <linux/mfd/wm831x/auxadc.h>
@@ -31,6 +32,8 @@ struct wm831x_power {
 	char usb_name[20];
 	char battery_name[20];
 	bool have_battery;
+	struct usb_charger *usb_charger;
+	struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In milliamps */
+static unsigned int wm831x_usb_limits[] = {
+	0,
+	2,
+	100,
+	500,
+	900,
+	1500,
+	1800,
+	550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+				   unsigned long limit, void *data)
+{
+	struct wm831x_power *wm831x_power = container_of(nb,
+							 struct wm831x_power,
+							 usb_notify);
+	int i, best;
+
+	/* Find the highest supported limit */
+	best = 0;
+	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+		if (limit >= wm831x_usb_limits[i] &&
+		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
+			best = i;
+	}
+
+	dev_dbg(wm831x_power->wm831x->dev,
+		"Limiting USB current to %dmA", wm831x_usb_limits[best]);
+
+	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+		        WM831X_USB_ILIM_MASK, best);
+
+	return 0;
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -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 v7 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Felipe Balbi]

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


Hi Baolin,

Baolin Wang <baolin.wang@linaro.org> writes:
> [ text/plain ]
> 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 v5:
>  - Remove the notifier chain things from the gadget and introduce one callback
>  function to report to the usb charger when the gadget state is changed.
>  - Flesh out the port type detection which combines the USB negotiation and
>  PMICs detection.
>  - Supply the notification mechanism to userspace when charger state is changed.
>  - Integrate with the vbus staff in the gadget API.
>  - Spilt up the functionality for userspace with one file per USB charger type.
>  - Rebase on "4.5-rc4".
>
> Baolin Wang (4):
>   gadget: Introduce the usb charger framework
>   gadget: Support for the usb charger framework
>   gadget: Integrate with the usb gadget supporting for usb charger
>   power: wm831x_power: Support USB charger current limit management

Thank you for resending this. I'll review it ASAP (probably tomorrow
when I get to the office. If I have time, still today)

-- 
balbi

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

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

[Felipe Balbi]

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


Hi Baolin,

Baolin Wang <baolin.wang@linaro.org> writes:
> [ text/plain ]
> 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 v5:
>  - Remove the notifier chain things from the gadget and introduce one callback
>  function to report to the usb charger when the gadget state is changed.
>  - Flesh out the port type detection which combines the USB negotiation and
>  PMICs detection.
>  - Supply the notification mechanism to userspace when charger state is changed.
>  - Integrate with the vbus staff in the gadget API.
>  - Spilt up the functionality for userspace with one file per USB charger type.
>  - Rebase on "4.5-rc4".
>
> Baolin Wang (4):
>   gadget: Introduce the usb charger framework
>   gadget: Support for the usb charger framework
>   gadget: Integrate with the usb gadget supporting for usb charger
>   power: wm831x_power: Support USB charger current limit management

Thank you for resending this. I'll review it ASAP (probably tomorrow
when I get to the office. If I have time, still today)

-- 
balbi

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

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

[Baolin Wang]

On 16 March 2016 at 19:48, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi Baolin,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> [ text/plain ]
>> 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 v5:
>>  - Remove the notifier chain things from the gadget and introduce one callback
>>  function to report to the usb charger when the gadget state is changed.
>>  - Flesh out the port type detection which combines the USB negotiation and
>>  PMICs detection.
>>  - Supply the notification mechanism to userspace when charger state is changed.
>>  - Integrate with the vbus staff in the gadget API.
>>  - Spilt up the functionality for userspace with one file per USB charger type.
>>  - Rebase on "4.5-rc4".
>>
>> Baolin Wang (4):
>>   gadget: Introduce the usb charger framework
>>   gadget: Support for the usb charger framework
>>   gadget: Integrate with the usb gadget supporting for usb charger
>>   power: wm831x_power: Support USB charger current limit management
>
> Thank you for resending this. I'll review it ASAP (probably tomorrow
> when I get to the office. If I have time, still today)

OK. Thanks:)

>
> --
> balbi



-- 
Baolin.wang
Best Regards

[PATCH v7 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 v5:
 - Remove the notifier chain things from the gadget and introduce one callback
 function to report to the usb charger when the gadget state is changed.
 - Flesh out the port type detection which combines the USB negotiation and
 PMICs detection.
 - Supply the notification mechanism to userspace when charger state is changed.
 - Integrate with the vbus staff in the gadget API.
 - Spilt up the functionality for userspace with one file per USB charger type.
 - Rebase on "4.4-rc7". 

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

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

-- 
1.7.9.5

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

[Baolin Wang]

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

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

Introduce a callback 'get_charger_type' which will implemented by
user for usb gadget operations to get the usb charger type.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..2727f01 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/usb_charger.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -226,6 +227,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");
 }
@@ -405,8 +409,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	mutex_unlock(&udc_lock);
 
+	ret = usb_charger_init(gadget);
+	if (ret)
+		goto err5;
+
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -481,6 +491,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 3d583a1..52c19b1 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/usb_charger.h>
 
 struct usb_ep;
 
@@ -560,6 +561,7 @@ struct usb_gadget_ops {
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
+	enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -632,6 +634,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	/* negotiate the power with the usb charger */
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -836,10 +840,17 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+	if (gadget->charger)
+		usb_charger_set_cur_limit_by_type(gadget->charger,
+				usb_charger_detect_type(gadget->charger), mA);
+
 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

[PATCH v7 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 v5:
 - Remove the notifier chain things from the gadget and introduce one callback
 function to report to the usb charger when the gadget state is changed.
 - Flesh out the port type detection which combines the USB negotiation and
 PMICs detection.
 - Supply the notification mechanism to userspace when charger state is changed.
 - Integrate with the vbus staff in the gadget API.
 - Spilt up the functionality for userspace with one file per USB charger type.

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

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

-- 
1.7.9.5

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

[Baolin Wang]

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

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

Introduce a callback 'get_charger_type' which will implemented by
user for usb gadget operations to get the usb charger type.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..2727f01 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/usb_charger.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -226,6 +227,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");
 }
@@ -405,8 +409,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	mutex_unlock(&udc_lock);
 
+	ret = usb_charger_init(gadget);
+	if (ret)
+		goto err5;
+
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -481,6 +491,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 3d583a1..52c19b1 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/usb_charger.h>
 
 struct usb_ep;
 
@@ -560,6 +561,7 @@ struct usb_gadget_ops {
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
+	enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -632,6 +634,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	/* negotiate the power with the usb charger */
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -836,10 +840,17 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+	if (gadget->charger)
+		usb_charger_set_cur_limit_by_type(gadget->charger,
+				usb_charger_detect_type(gadget->charger), mA);
+
 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

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

[kbuild test robot]

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

Hi Baolin,

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v4.4-rc4 next-20151208]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/gadget-Introduce-the-usb-charger-framework/20151208-163942
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: m68k-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   ERROR: "usb_charger_init" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_plug_by_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_exit" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/libcomposite.ko] undefined!
   ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 34882 bytes --]

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

[kbuild test robot]

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

Hi Baolin,

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v4.4-rc4 next-20151208]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/gadget-Introduce-the-usb-charger-framework/20151208-163942
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: m68k-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   ERROR: "usb_charger_init" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_plug_by_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_exit" [drivers/usb/gadget/udc/udc-core.ko] undefined!
   ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/libcomposite.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/libcomposite.ko] undefined!
   ERROR: "usb_charger_set_cur_limit_by_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!
>> ERROR: "usb_charger_detect_type" [drivers/usb/gadget/legacy/gadgetfs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 34882 bytes --]