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

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

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should.

Providing a standard framework for doing this in the kernel.

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

Changes since v3:
 - Re-order the patch and split the patch to avoid breaking build.
 - Other modifications.

Baolin Wang (5):
  gadget: Introduce the notifier functions
  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      | 538 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/udc-core.c |  40 +++
 include/linux/mfd/wm831x/pdata.h  |   3 +
 include/linux/usb/gadget.h        |  20 ++
 include/linux/usb/usb_charger.h   | 138 ++++++++++
 8 files changed, 816 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

-- 
1.9.1

[PATCH v4 1/5] gadget: Introduce the notifier functions

[Baolin Wang]

The usb charger framework is based on usb gadget. The usb charger
need to be notified the state changing of usb gadget to confirm the
usb charger state.

Thus this patch adds a notifier mechanism for usb gadget to report a
event to usb charger when the usb gadget state is changed.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..4238fc3 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_register(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* ------------------------------------------------------------------------- */
 
 /**
@@ -226,6 +252,10 @@ static void usb_gadget_state_work(struct work_struct *work)
 	struct usb_gadget *gadget = work_to_gadget(work);
 	struct usb_udc *udc = gadget->udc;
 
+	mutex_lock(&gadget->lock);
+	raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
+	mutex_unlock(&gadget->lock);
+
 	if (udc)
 		sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -364,6 +394,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	dev_set_name(&gadget->dev, "gadget");
 	INIT_WORK(&gadget->work, usb_gadget_state_work);
+	RAW_INIT_NOTIFIER_HEAD(&gadget->nh);
+	mutex_init(&gadget->lock);
 	gadget->dev.parent = parent;
 
 #ifdef	CONFIG_HAS_DMA
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c14a69b..755e8bc 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -609,6 +609,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	struct raw_notifier_head	nh;
+	struct mutex			lock;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -1183,6 +1185,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget *gadget,
 
 /*-------------------------------------------------------------------------*/
 
+/**
+ * Register a notifiee to get notified by any attach status changes from
+ * the usb gadget
+ */
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
+
+/* Unregister a notifiee from the usb gadget */
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
 /* utility to set gadget state properly */
 
 extern void usb_gadget_set_state(struct usb_gadget *gadget,
-- 
1.9.1

[PATCH v4 2/5] gadget: Introduce the usb charger framework

[Baolin Wang]

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

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

The usb charger will register notifiees on the usb gadget or the extcon
device to get notified the usb charger state.

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

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

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index bcf83c0..3d2b959 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
 	   a module parameter as well.
 	   If unsure, say 2.
 
+config USB_CHARGER
+	bool "USB charger support"
+	help
+	  The usb charger driver based on the usb gadget that makes an
+	  enhancement to a power driver which can set the current limitation
+	  when the usb charger is added or removed.
+
 source "drivers/usb/gadget/udc/Kconfig"
 
 #
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 598a67d..1e421c1 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
 libcomposite-y			+= composite.o functions.o configfs.o u_f.o
 
 obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
+obj-$(CONFIG_USB_CHARGER)	+= charger.o
diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
new file mode 100644
index 0000000..35b46c1
--- /dev/null
+++ b/drivers/usb/gadget/charger.c
@@ -0,0 +1,529 @@
+/*
+ * 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>
+
+#define DEFAULT_CUR_PROTECT	(50)
+#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+
+static DEFINE_IDA(usb_charger_ida);
+static struct bus_type usb_charger_subsys = {
+	.name           = "usb-charger",
+	.dev_name       = "usb-charger",
+};
+
+static struct usb_charger *dev_to_uchger(struct device *udev)
+{
+	return container_of(udev, struct usb_charger, dev);
+}
+
+static ssize_t cur_limit_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n",
+			 uchger->cur_limit.sdp_cur_limit,
+			 uchger->cur_limit.dcp_cur_limit,
+			 uchger->cur_limit.cdp_cur_limit,
+			 uchger->cur_limit.aca_cur_limit);
+}
+
+static ssize_t cur_limit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	struct usb_charger_cur_limit cur;
+	int ret;
+
+	ret = sscanf(buf, "%d %d %d %d",
+		     &cur.sdp_cur_limit, &cur.dcp_cur_limit,
+		     &cur.cdp_cur_limit, &cur.aca_cur_limit);
+	if (ret == 0)
+		return -EINVAL;
+
+	ret = usb_charger_set_cur_limit(uchger, &cur);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(cur_limit);
+
+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_cur_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);
+}
+
+/*
+ * usb_charger_get() - Reference a usb charger.
+ * @uchger - usb charger
+ */
+struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
+}
+
+/*
+ * usb_charger_put() - Dereference a usb charger.
+ * @uchger - charger to release
+ */
+void usb_charger_put(struct usb_charger *uchger)
+{
+	if (uchger)
+		put_device(&uchger->dev);
+}
+
+/*
+ * usb_charger_register_notify() - Register a notifiee to get notified by
+ * any attach status changes from the usb charger detection.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be registered.
+ */
+int usb_charger_register_notify(struct usb_charger *uchger,
+				struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
+
+	/* Generate an initial notify so users start in the right state */
+	if (!ret) {
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					usb_charger_get_cur_limit(uchger),
+					uchger);
+	}
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+
+/*
+ * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be unregistered.
+ */
+int usb_charger_unregister_notify(struct usb_charger *uchger,
+				  struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+
+/*
+ * usb_charger_detect_type() - Get the usb charger type by the callback
+ * which is implemented by gadget operations.
+ * @uchger - the usb charger device.
+ *
+ * return the usb charger type.
+ */
+enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	return uchger->type;
+}
+
+/*
+ * usb_charger_set_cur_limit() - Set the current limitation.
+ * @uchger - the usb charger device.
+ * @cur_limit_set - the current limitation.
+ */
+int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set)
+{
+	if (!uchger || !cur_limit_set)
+		return -EINVAL;
+
+	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
+	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
+	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
+	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
+	return 0;
+}
+
+/*
+ * usb_charger_get_cur_limit() - Get the current limitation by
+ * different usb charger type.
+ * @uchger - the usb charger device.
+ *
+ * return the current limitation to set.
+ */
+unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	enum usb_charger_type uchger_type = uchger->type;
+	unsigned int cur_limit;
+
+	switch (uchger_type) {
+	case SDP_TYPE:
+		cur_limit = uchger->cur_limit.sdp_cur_limit;
+		break;
+	case DCP_TYPE:
+		cur_limit = uchger->cur_limit.dcp_cur_limit;
+		break;
+	case CDP_TYPE:
+		cur_limit = uchger->cur_limit.cdp_cur_limit;
+		break;
+	case ACA_TYPE:
+		cur_limit = uchger->cur_limit.aca_cur_limit;
+		break;
+	default:
+		return 0;
+	}
+
+	return cur_limit;
+}
+
+/*
+ * usb_charger_notifier_others() - It will notify other device registered
+ * on usb charger when the usb charger state is changed.
+ * @uchger - the usb charger device.
+ * @state - the state of the usb charger.
+ */
+static void
+usb_charger_notify_others(struct usb_charger *uchger,
+			  enum usb_charger_state state)
+{
+	mutex_lock(&uchger->lock);
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+			usb_charger_get_cur_limit(uchger),
+			uchger);
+		break;
+	case USB_CHARGER_REMOVE:
+		uchger->type = UNKNOWN_TYPE;
+		raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
+		break;
+	default:
+		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
+			 state);
+		break;
+	}
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * usb_charger_plug_by_extcon() - The notifier call function which is registered
+ * on the extcon device.
+ * @nb - the notifier block that notified by extcon device.
+ * @state - the extcon device state.
+ * @data - here specify a extcon device.
+ *
+ * return the notify flag.
+ */
+static int
+usb_charger_plug_by_extcon(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_charger_nb *extcon_nb =
+		container_of(nb, struct usb_charger_nb, nb);
+	struct usb_charger *uchger = extcon_nb->uchger;
+	enum usb_charger_state uchger_state;
+
+	if (!uchger)
+		return NOTIFY_BAD;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger is added or removed
+	 * with detecting by extcon device.
+	 */
+	if (state)
+		uchger_state = USB_CHARGER_PRESENT;
+	else
+		uchger_state = USB_CHARGER_REMOVE;
+
+	usb_charger_notify_others(uchger, uchger_state);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * usb_charger_plug_by_gadget() - Set the usb charger current limitation
+ * according to the usb gadget device state.
+ * @nb - the notifier block that notified by usb gadget device.
+ * @state - the usb gadget state.
+ * @data - here specify a usb gadget device.
+ */
+static int
+usb_charger_plug_by_gadget(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_charger *uchger = NULL;
+	enum usb_charger_state uchger_state;
+
+	if (!uchger)
+		return NOTIFY_BAD;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger state is changed
+	 * with detecting by usb gadget state.
+	 */
+	if (uchger->old_gadget_state != state) {
+		uchger->old_gadget_state = state;
+
+		if (state >= USB_STATE_ATTACHED)
+			uchger_state = USB_CHARGER_PRESENT;
+		else if (state == USB_STATE_NOTATTACHED)
+			uchger_state = USB_CHARGER_REMOVE;
+		else
+			uchger_state = USB_CHARGER_DEFAULT;
+
+		usb_charger_notify_others(uchger, uchger_state);
+	}
+
+	return NOTIFY_OK;
+}
+
+static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
+{
+	struct usb_charger **r = res;
+
+	if (WARN_ON(!r || !*r))
+		return 0;
+
+	return *r == data;
+}
+
+static void usb_charger_release(struct device *dev)
+{
+	struct usb_charger *uchger = dev_get_drvdata(dev);
+
+	kfree(uchger);
+}
+
+/*
+ * 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;
+	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;
+
+	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);
+	}
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = ugadget->state;
+	uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
+	usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
+
+	/* register a new usb charger */
+	ret = usb_charger_register(&ugadget->dev, uchger);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
+	kfree(uchger);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger = NULL;
+
+	if (!uchger)
+		return -EINVAL;
+
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+
+	return usb_charger_unregister(uchger);
+}
+
+static int __init usb_charger_sysfs_init(void)
+{
+	return subsys_system_register(&usb_charger_subsys, NULL);
+}
+core_initcall(usb_charger_sysfs_init);
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
+MODULE_DESCRIPTION("USB charger driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
new file mode 100644
index 0000000..99bc26f
--- /dev/null
+++ b/include/linux/usb/usb_charger.h
@@ -0,0 +1,138 @@
+#ifndef __LINUX_USB_CHARGER_H__
+#define __LINUX_USB_CHARGER_H__
+
+/* USB charger type:
+ * SDP (Standard Downstream Port)
+ * DCP (Dedicated Charging Port)
+ * CDP (Charging Downstream Port)
+ * ACA (Accessory Charger Adapters)
+ */
+enum usb_charger_type {
+	UNKNOWN_TYPE,
+	SDP_TYPE,
+	DCP_TYPE,
+	CDP_TYPE,
+	ACA_TYPE,
+};
+
+/* USB charger state */
+enum usb_charger_state {
+	USB_CHARGER_DEFAULT,
+	USB_CHARGER_PRESENT,
+	USB_CHARGER_REMOVE,
+};
+
+/* 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;
+	struct notifier_block	gadget_nb;
+
+	/* current limitation */
+	struct usb_charger_cur_limit	cur_limit;
+};
+
+#ifdef CONFIG_USB_CHARGER
+extern struct usb_charger *usb_charger_find_by_name(const char *name);
+
+extern struct usb_charger *usb_charger_get(struct usb_charger *uchger);
+extern void usb_charger_put(struct usb_charger *uchger);
+
+extern int usb_charger_register_notify(struct usb_charger *uchger,
+				       struct notifier_block *nb);
+extern int usb_charger_unregister_notify(struct usb_charger *uchger,
+					 struct notifier_block *nb);
+
+extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
+				struct usb_charger_cur_limit *cur_limit_set);
+extern unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+extern enum usb_charger_type usb_charger_detect_type(struct usb_charger *uchger);
+
+extern int usb_charger_init(struct usb_gadget *ugadget);
+extern int usb_charger_exit(struct usb_gadget *ugadget);
+#else
+static inline struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return NULL;
+}
+
+static inline void usb_charger_put(struct usb_charger *uchger)
+{
+}
+
+static inline int
+usb_charger_register_notify(struct usb_charger *uchger,
+			    struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_unregister_notify(struct usb_charger *uchger,
+			      struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit(struct usb_charger *uchger,
+			  struct usb_charger_cur_limit *cur_limit_set)
+{
+	return 0;
+}
+
+static inline unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	return UNKNOWN_TYPE;
+}
+
+static inline int usb_charger_init(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+static inline int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+#endif
+
+#endif /* __LINUX_USB_CHARGER_H__ */
-- 
1.9.1

[PATCH v4 3/5] 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.

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 | 8 ++++++++
 include/linux/usb/gadget.h        | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 4238fc3..370376e 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
@@ -437,8 +438,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);
@@ -513,6 +520,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 755e8bc..451ad32 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -537,6 +537,7 @@ struct usb_gadget_ops {
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
+	enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -611,6 +612,7 @@ struct usb_gadget {
 	struct usb_otg_caps		*otg_caps;
 	struct raw_notifier_head	nh;
 	struct mutex			lock;
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
-- 
1.9.1

[PATCH v4 4/5] 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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
index 35b46c1..a919f38 100644
--- a/drivers/usb/gadget/charger.c
+++ b/drivers/usb/gadget/charger.c
@@ -181,6 +181,13 @@ int usb_charger_unregister_notify(struct usb_charger *uchger,
 enum usb_charger_type
 usb_charger_detect_type(struct usb_charger *uchger)
 {
+	if (uchger->gadget && uchger->gadget->ops
+	    && uchger->gadget->ops->get_charger_type)
+		uchger->type =
+			uchger->gadget->ops->get_charger_type(uchger->gadget);
+	else
+		uchger->type = UNKNOWN_TYPE;
+
 	return uchger->type;
 }
 
@@ -313,7 +320,8 @@ static int
 usb_charger_plug_by_gadget(struct notifier_block *nb,
 			   unsigned long state, void *data)
 {
-	struct usb_charger *uchger = NULL;
+	struct usb_gadget *gadget = (struct usb_gadget *)data;
+	struct usb_charger *uchger = gadget->charger;
 	enum usb_charger_state uchger_state;
 
 	if (!uchger)
@@ -480,6 +488,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;
 	uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
 	usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
@@ -503,7 +512,7 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-	struct usb_charger *uchger = NULL;
+	struct usb_charger *uchger = ugadget->charger;
 
 	if (!uchger)
 		return -EINVAL;
-- 
1.9.1

[PATCH v4 5/5] 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 db11ae6..33ae27a 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
  *********************************************************************/
@@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+		power->usb_charger =
+			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
+		if (IS_ERR(power->usb_charger)) {
+			ret = PTR_ERR(power->usb_charger);
+			dev_err(&pdev->dev,
+				"Failed to find USB gadget: %d\n", ret);
+			goto err_bat_irq;
+		}
+
+		power->usb_notify.notifier_call = wm831x_usb_limit_change;
+
+		ret = usb_charger_register_notify(power->usb_charger,
+						  &power->usb_notify);
+		if (ret != 0) {
+			dev_err(&pdev->dev,
+				"Failed to register notifier: %d\n", ret);
+			goto err_usb_charger;
+		}
+	}
+
 	return ret;
 
+err_usb_charger:
+	/* 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.9.1

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> The usb charger framework is based on usb gadget. The usb charger
> need to be notified the state changing of usb gadget to confirm the
> usb charger state.
> 
> Thus this patch adds a notifier mechanism for usb gadget to report a
> event to usb charger when the usb gadget state is changed.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index f660afb..4238fc3 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
>  
> +int usb_gadget_register_notify(struct usb_gadget *gadget,
> +			       struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> +	mutex_unlock(&gadget->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> +
> +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> +				 struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);

Greg, this is the kind of thing I wanted to avoid adding more of.

I was wondering if you would accept subsystems using kdbus for
this sort of notification. I'm okay waiting for kdbus for another
couple merge windows (if we have to) before that's merged, but
if we take this raw notifier approach now, we will end up having
to support it forever.

Also, because soon enough we will have to support USB Power Delivery
with Type C connector, this is bound to change in the coming months.

Frankly, I wanted all of this to be decided in userland with the
kernel just providing notification and basic safety checks (we don't
want to allow a bogus userspace daemon frying anybody's devices).

How would you feel about that ?

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).

What's the advantage of pushing this to userspace?  By the time we
provide enough discoverability to enable userspace to configure itself
it seems like we'd have enough information to do the job anyway.

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

Hi,

On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> 
> > Frankly, I wanted all of this to be decided in userland with the
> > kernel just providing notification and basic safety checks (we don't
> > want to allow a bogus userspace daemon frying anybody's devices).
> 
> What's the advantage of pushing this to userspace?  By the time we
> provide enough discoverability to enable userspace to configure itself
> it seems like we'd have enough information to do the job anyway.

you're going to be dealing with a setup where each vendor does one thing
differently. Some will have it all in the SoC part of a single IP (dwc3 can be
configured that way), some will push it out to companion IC, some might even use
some mostly discrete setup and so on...

We're gonna be dealing with a decision that involves information from multiple
subsystems (USB, regulator, hwmon, power supply to name a few).

We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
just plain difficult. To make matters worse, N900 had two USB PHYs, one for
actual runtime use and another just for detecting the charger type on the other
end.

On top of all that, we still need to figure out what to do when a particular
configuration gets chosen, and what to do when the bus goes into the different
suspend levels.

It's going to be a lot of policy getting added to the kernel. On top of all
that, when Type C and power delivery is finally a thing, there will an entire
new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
legacy connectors) which we will have to add to the kernel. And different
devices will support different charging profiles (there are at least 6 of those,
IIRC).

With all these different things going on, it's best to do what e.g. NFC folks
did. Just a small set of hooks in the kernel, but actual implementation done by
a userspace daemon. This makes it even easier for middleware development since
we can provide a higher level API for middleware to talk to the charging daemon.

Another benefit is that this charging daemon can also give hints to power
management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
to performance governor safely even though battery is rather low.

Anyway, there's really a lot that needs to happen and shuving it all in the
kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
requirements in the kernel and let a userspace daemon (which we should certainly
provide a reference implementation) figure out what to do. This might be better
for things like Chromebooks and Android phones too which might make completely
different decisions given a certain charging profile.

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> > 
> > > Frankly, I wanted all of this to be decided in userland with the
> > > kernel just providing notification and basic safety checks (we don't
> > > want to allow a bogus userspace daemon frying anybody's devices).
> > 
> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.
> 
> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use

oh, and as for dwc3 itself: it *can* be configured that way, but all those
charging blocks are optional :-) So you will even have setups where the very
same IP works differently because SoC vendor A configured it differently from
SoC vendor B.

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Greg KH]

On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> > The usb charger framework is based on usb gadget. The usb charger
> > need to be notified the state changing of usb gadget to confirm the
> > usb charger state.
> > 
> > Thus this patch adds a notifier mechanism for usb gadget to report a
> > event to usb charger when the usb gadget state is changed.
> > 
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
> >  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > index f660afb..4238fc3 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
> >  }
> >  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> >  
> > +int usb_gadget_register_notify(struct usb_gadget *gadget,
> > +			       struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> > +	mutex_unlock(&gadget->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> > +
> > +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> > +				 struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> 
> Greg, this is the kind of thing I wanted to avoid adding more of.
> 
> I was wondering if you would accept subsystems using kdbus for
> this sort of notification. I'm okay waiting for kdbus for another
> couple merge windows (if we have to) before that's merged, but
> if we take this raw notifier approach now, we will end up having
> to support it forever.

kdbus is userspace <-> userspace messages, it doesn't do kernel <->
userspace messages, sorry.

> Also, because soon enough we will have to support USB Power Delivery
> with Type C connector, this is bound to change in the coming months.
> 
> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).
> 
> How would you feel about that ?

I agree I don't like new notifier chains being added, but as this is all
in-kernel, we can change the api in the future and there would not be a
problem, right?

And yeah, I'm worried about the USB Power delivery patches, I haven't
seen them yet, but I hear about different groups doing different things
here, and that worries me.

thanks,

greg k-h

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> > > Frankly, I wanted all of this to be decided in userland with the
> > > kernel just providing notification and basic safety checks (we don't
> > > want to allow a bogus userspace daemon frying anybody's devices).

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.


> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use
> some mostly discrete setup and so on...

Right, and that was exactly what this was supposed to be all about when
I was discussing this originally with Baolin (who's on holiday until
sometime next week BTW, hence me answering).

> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).

Sure, that was part of the idea here - provide a central point
representing the logical port where we can aggregate all the information
we get in and distribute it to consumers.  

> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the other
> end.

Can you elaborate on what the difficulties are and how punting to
userspace helps?  If anything I'd expect punting to userspace to make
things more difficult, if nothing else it means we need to get whatever
userspace component deployed in all relevant userspace stacks with
appropriate configuration in order to do things.  

We do punt a lot of configuration to userspace for audio because there
are substantial device specific policy elements in the configuration and
it's a far from painless experience getting the code and configuration
deployed where people need it, definitely a not great for users.

> On top of all that, we still need to figure out what to do when a particular
> configuration gets chosen, and what to do when the bus goes into the different
> suspend levels.

> It's going to be a lot of policy getting added to the kernel. On top of all
> that, when Type C and power delivery is finally a thing, there will an entire
> new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
> legacy connectors) which we will have to add to the kernel. And different
> devices will support different charging profiles (there are at least 6 of those,
> IIRC).

Yes, USB C was part of the thinking with starting this work - it's going
to make ensuring that the system is paying attention to limits much more
important.  I think there's two things here.  One is working out how the
system is glued together and the other thing is working out what to do
with that system.  

I think that where we can do it the bit where work out how the various
components are connected and aggregate their functionality together is
definitely a kernel job.  It means that userspace doesn't need to worry
about implementation details of the particular system and instead gets a
consistent, standard view of the device (in this case a USB port and how
much power we can draw from it).

For the policy stuff we do want to have userspace be able to control
things but we need to think about how to do that - for example does the
entire flow need to be in userspace, or can we just expose settings
which userspace can manage?

> With all these different things going on, it's best to do what e.g. NFC folks
> did. Just a small set of hooks in the kernel, but actual implementation done by
> a userspace daemon. This makes it even easier for middleware development since
> we can provide a higher level API for middleware to talk to the charging daemon.

I'm not sure that the NFC model is working well for everyone - it's OK
if you happen to be running Android but if you aren't you're often left
sitting there working out what to do with an Android HAL.  We can do the
middleware thing without going quite that far, we do already have power
management daemons in userspace even on desktop (GNOME has upowerd for
example).

> Another benefit is that this charging daemon can also give hints to power
> management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
> to performance governor safely even though battery is rather low.

We definitely want userspace to be able to see the current state, even
if it just passes it straight on to the user it's a common part of UIs
on both desktop and mobile operating systems.

> Anyway, there's really a lot that needs to happen and shuving it all in the
> kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
> requirements in the kernel and let a userspace daemon (which we should certainly
> provide a reference implementation) figure out what to do. This might be better
> for things like Chromebooks and Android phones too which might make completely
> different decisions given a certain charging profile.

Saying we don't want to have absolutely everything in kernel space
doesn't mean we should have nothing at all there, doing that seems like
going too far in the other direction.

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

Hi,

On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:
> On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> 
> > > > Frankly, I wanted all of this to be decided in userland with the
> > > > kernel just providing notification and basic safety checks (we don't
> > > > want to allow a bogus userspace daemon frying anybody's devices).
> 
> > > What's the advantage of pushing this to userspace?  By the time we
> > > provide enough discoverability to enable userspace to configure itself
> > > it seems like we'd have enough information to do the job anyway.
> 
> 
> > you're going to be dealing with a setup where each vendor does one thing
> > differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> > configured that way), some will push it out to companion IC, some might even use
> > some mostly discrete setup and so on...
> 
> Right, and that was exactly what this was supposed to be all about when
> I was discussing this originally with Baolin (who's on holiday until
> sometime next week BTW, hence me answering).
> 
> > We're gonna be dealing with a decision that involves information from multiple
> > subsystems (USB, regulator, hwmon, power supply to name a few).
> 
> Sure, that was part of the idea here - provide a central point
> representing the logical port where we can aggregate all the information
> we get in and distribute it to consumers.  
> 
> > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> > just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> > actual runtime use and another just for detecting the charger type on the other
> > end.
> 
> Can you elaborate on what the difficulties are and how punting to
> userspace helps?  If anything I'd expect punting to userspace to make

the difficulty was mainly making sure all involved parties talk the same
language. I mean, you plug cable and detect charger (this is usually done by the
PMIC or by a BQ-type device - probably done at drivers/power_supply).

First difficulty comes right here. Power_supply notifies that we're attached to
a charger (sometimes it can't differentiate a host/hub charger from a wall
charger), a few ms later you get a notification from the gadget that it got
enumerated with a 500mA configuration. Depending on the order of things, your
load will be configured either to 2A (maximum host/hub charger current) or
500mA. Yes, this can be mitigated :-)

Focussing on this alone, you can have as much as 4 different subsystems
involved, all throwing raw_notifiers at each other. Now each of these subsystems
need to maintain their own state machine about what notification we have
received and what are the valid next states.

I would rather have userspace be the single place getting notifications because
then we solve these details at a single location.

> Things more difficult, if nothing else it means we need to get whatever
> userspace component deployed in all relevant userspace stacks with
> appropriate configuration in order to do things.

but that will be a thing for the kernel too. We will have to deploy this new
kernel component in all relevant stacks with appropriate configuration in order
to do things :-) No changes there.

> We do punt a lot of configuration to userspace for audio because there
> are substantial device specific policy elements in the configuration and
> it's a far from painless experience getting the code and configuration
> deployed where people need it, definitely a not great for users.

it's the same for this situation. We will have a ton of device specific
configuration, specially with power delivery class, billboard class, the
alternate modes and so on. If we already do this part in userland, adding all
those extras will be, IMO, far simpler.

> > On top of all that, we still need to figure out what to do when a particular
> > configuration gets chosen, and what to do when the bus goes into the different
> > suspend levels.
> 
> > It's going to be a lot of policy getting added to the kernel. On top of all
> > that, when Type C and power delivery is finally a thing, there will an entire
> > new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
> > legacy connectors) which we will have to add to the kernel. And different
> > devices will support different charging profiles (there are at least 6 of those,
> > IIRC).
> 
> Yes, USB C was part of the thinking with starting this work - it's going
> to make ensuring that the system is paying attention to limits much more
> important.  I think there's two things here.  One is working out how the
> system is glued together and the other thing is working out what to do
> with that system.

right, and IMO, what to do should be a decision made outside of the kernel as
long as kernel provides safety.

> I think that where we can do it the bit where work out how the various
> components are connected and aggregate their functionality together is
> definitely a kernel job.  It means that userspace doesn't need to worry
> about implementation details of the particular system and instead gets a
> consistent, standard view of the device (in this case a USB port and how
> much power we can draw from it).

Actually, I was thinking about it in a more granular fashion. Kernel exposes a
charger/power_supply, a few regulators, a UDC view and this single userspace
daemon figures out how to tie those together.

The view here isn't really a USB port, it's just a source of power. But how much
power we can draw from it depends on, possibly, a ton of different chips and
different notifications.

> For the policy stuff we do want to have userspace be able to control
> things but we need to think about how to do that - for example does the
> entire flow need to be in userspace, or can we just expose settings
> which userspace can manage?

considering the amount of different designs that are already out there and all
the extras that are going to show up due to type-c, I think we'd be better off
exposing as much to userspace as possible.

> > With all these different things going on, it's best to do what e.g. NFC folks
> > did. Just a small set of hooks in the kernel, but actual implementation done by
> > a userspace daemon. This makes it even easier for middleware development since
> > we can provide a higher level API for middleware to talk to the charging daemon.
> 
> I'm not sure that the NFC model is working well for everyone - it's OK
> if you happen to be running Android but if you aren't you're often left
> sitting there working out what to do with an Android HAL.  We can do the

NFC works pretty well for neard, afaict. And without android.

> middleware thing without going quite that far, we do already have power
> management daemons in userspace even on desktop (GNOME has upowerd for
> example).

right

> > Another benefit is that this charging daemon can also give hints to power
> > management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
> > to performance governor safely even though battery is rather low.
> 
> We definitely want userspace to be able to see the current state, even
> if it just passes it straight on to the user it's a common part of UIs
> on both desktop and mobile operating systems.
> 
> > Anyway, there's really a lot that needs to happen and shuving it all in the
> > kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
> > requirements in the kernel and let a userspace daemon (which we should certainly
> > provide a reference implementation) figure out what to do. This might be better
> > for things like Chromebooks and Android phones too which might make completely
> > different decisions given a certain charging profile.
> 
> Saying we don't want to have absolutely everything in kernel space
> doesn't mean we should have nothing at all there, doing that seems like
> going too far in the other direction.

we _have_ to have something in the kernel :-) I'm arguing that we might not want
as much as you think we do :-)

The real difficulty here is coming up with an interface which we're agreeing to
supporting for the next 30 years. Whatever that is, as soon as it gets exposed
to userland, we will have to support it.

And this another reason I think a more granular approach is better, as it allows
us to easily add more pieces as they come along.

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

Hi,

On Fri, Oct 02, 2015 at 07:41:07AM +0200, Greg KH wrote:
> On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> > On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> > > The usb charger framework is based on usb gadget. The usb charger
> > > need to be notified the state changing of usb gadget to confirm the
> > > usb charger state.
> > > 
> > > Thus this patch adds a notifier mechanism for usb gadget to report a
> > > event to usb charger when the usb gadget state is changed.
> > > 
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
> > >  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
> > >  2 files changed, 50 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > > index f660afb..4238fc3 100644
> > > --- a/drivers/usb/gadget/udc/udc-core.c
> > > +++ b/drivers/usb/gadget/udc/udc-core.c
> > > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> > >  
> > > +int usb_gadget_register_notify(struct usb_gadget *gadget,
> > > +			       struct notifier_block *nb)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&gadget->lock);
> > > +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> > > +	mutex_unlock(&gadget->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> > > +
> > > +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> > > +				 struct notifier_block *nb)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&gadget->lock);
> > > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> > 
> > Greg, this is the kind of thing I wanted to avoid adding more of.
> > 
> > I was wondering if you would accept subsystems using kdbus for
> > this sort of notification. I'm okay waiting for kdbus for another
> > couple merge windows (if we have to) before that's merged, but
> > if we take this raw notifier approach now, we will end up having
> > to support it forever.
> 
> kdbus is userspace <-> userspace messages, it doesn't do kernel <->
> userspace messages, sorry.

oh well, tough luck :-)

having a more well-constructed notification method to userspace would be a
little better, but I guess we will have to resort to sysfs_notify() or something
like that.

> > Also, because soon enough we will have to support USB Power Delivery
> > with Type C connector, this is bound to change in the coming months.
> > 
> > Frankly, I wanted all of this to be decided in userland with the
> > kernel just providing notification and basic safety checks (we don't
> > want to allow a bogus userspace daemon frying anybody's devices).
> > 
> > How would you feel about that ?
> 
> I agree I don't like new notifier chains being added, but as this is all
> in-kernel, we can change the api in the future and there would not be a
> problem, right?

not necessarily. This will, eventually, get exposed to userspace. At minimum for
adding eye candy to the UI. Chaging battery icon or whatever. The danger here is
that when that something gets exposed, we will have to support it for the next
30 years :-)

> And yeah, I'm worried about the USB Power delivery patches, I haven't
> seen them yet, but I hear about different groups doing different things
> here, and that worries me.

my worries exactly :-) This is why I'm looking for a flexible enough approach
which puts as little into the kernel as necessary.

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:

> > Can you elaborate on what the difficulties are and how punting to
> > userspace helps?  If anything I'd expect punting to userspace to make

> the difficulty was mainly making sure all involved parties talk the same
> language. I mean, you plug cable and detect charger (this is usually done by the
> PMIC or by a BQ-type device - probably done at drivers/power_supply).

> First difficulty comes right here. Power_supply notifies that we're attached to
> a charger (sometimes it can't differentiate a host/hub charger from a wall
> charger), a few ms later you get a notification from the gadget that it got
> enumerated with a 500mA configuration. Depending on the order of things, your
> load will be configured either to 2A (maximum host/hub charger current) or
> 500mA. Yes, this can be mitigated :-)

> Focussing on this alone, you can have as much as 4 different subsystems
> involved, all throwing raw_notifiers at each other. Now each of these subsystems
> need to maintain their own state machine about what notification we have
> received and what are the valid next states.

OK, so part of the goal here was to outsource all the state machines to
some generic code - what you're describing here is definitely a problem
and the idea was to provide a central place that has the logic and makes
the decisions about what we should be doing.  If we don't have that it's
going to get messy.

> I would rather have userspace be the single place getting notifications because
> then we solve these details at a single location.

No argument on the place, making that place be userspace I'm less sold
on.

> > Things more difficult, if nothing else it means we need to get whatever
> > userspace component deployed in all relevant userspace stacks with
> > appropriate configuration in order to do things.

> but that will be a thing for the kernel too. We will have to deploy this new
> kernel component in all relevant stacks with appropriate configuration in order
> to do things :-) No changes there.

Sure, but it's one project which is developed entirely in the open -
that's a lot more manageable.

> > We do punt a lot of configuration to userspace for audio because there
> > are substantial device specific policy elements in the configuration and
> > it's a far from painless experience getting the code and configuration
> > deployed where people need it, definitely a not great for users.

> it's the same for this situation. We will have a ton of device specific
> configuration, specially with power delivery class, billboard class, the
> alternate modes and so on. If we already do this part in userland, adding all
> those extras will be, IMO, far simpler.

Can you elaborate a bit?  As far as I can tell all those are still in
the generic USB space rather than the sort of device specifics I'm
thinking of.

The things we've got with audio are a combination of tuning to taste and
(even more importantly) very different ideas about what you want to do
with an audio subsystem that influence how you set things up in a given
use case.

> > I think that where we can do it the bit where work out how the various
> > components are connected and aggregate their functionality together is
> > definitely a kernel job.  It means that userspace doesn't need to worry
> > about implementation details of the particular system and instead gets a
> > consistent, standard view of the device (in this case a USB port and how
> > much power we can draw from it).

> Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> charger/power_supply, a few regulators, a UDC view and this single userspace
> daemon figures out how to tie those together.

That sounds worrying - it means that new hardware support needs
supporting in every userspace (it seems inevitable that there will be at
least some reimplementations because that's fun) which gets old really
fast, and every userspace needs to understand every topology.

> The view here isn't really a USB port, it's just a source of power. But how much
> power we can draw from it depends on, possibly, a ton of different chips and
> different notifications.

Right, yes - it's the power input/output associated with the USB port.
The USB port is just the physical thing users can see so it's a good way
to label it.  That could mean that we ought to move the aggregation bit
into the power supply code of course, but then a lot of the decisions
are going to be specific to USB.

> > For the policy stuff we do want to have userspace be able to control
> > things but we need to think about how to do that - for example does the
> > entire flow need to be in userspace, or can we just expose settings
> > which userspace can manage?

> considering the amount of different designs that are already out there and all
> the extras that are going to show up due to type-c, I think we'd be better off
> exposing as much to userspace as possible.

How different are the end goals of these designs really going to be
though - that's more of the question for me?  Part of what the kernel is
doing is hiding implementation details from userspace.  I'd expect
userspace to be interested in things like the maximum current allowed in
or out in a given mode rather than the details of how that is accomplished.

> > I'm not sure that the NFC model is working well for everyone - it's OK
> > if you happen to be running Android but if you aren't you're often left
> > sitting there working out what to do with an Android HAL.  We can do the

> NFC works pretty well for neard, afaict. And without android.

Ah, that looks better than it was now - it looks like they're providing
a reasonable hardware abstraction for messaging.  Still, there's other
examples of this model that are less successful where the kernel
interface isn't generic.

> The real difficulty here is coming up with an interface which we're agreeing to
> supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> to userland, we will have to support it.

To me that sounds like an argument for hiding things :)

> And this another reason I think a more granular approach is better, as it allows
> us to easily add more pieces as they come along.

OTOH it's more work for the system as a whole.

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:
> 
> > > Can you elaborate on what the difficulties are and how punting to
> > > userspace helps?  If anything I'd expect punting to userspace to make
> 
> > the difficulty was mainly making sure all involved parties talk the same
> > language. I mean, you plug cable and detect charger (this is usually done by the
> > PMIC or by a BQ-type device - probably done at drivers/power_supply).
> 
> > First difficulty comes right here. Power_supply notifies that we're attached to
> > a charger (sometimes it can't differentiate a host/hub charger from a wall
> > charger), a few ms later you get a notification from the gadget that it got
> > enumerated with a 500mA configuration. Depending on the order of things, your
> > load will be configured either to 2A (maximum host/hub charger current) or
> > 500mA. Yes, this can be mitigated :-)
> 
> > Focussing on this alone, you can have as much as 4 different subsystems
> > involved, all throwing raw_notifiers at each other. Now each of these subsystems
> > need to maintain their own state machine about what notification we have
> > received and what are the valid next states.
> 
> OK, so part of the goal here was to outsource all the state machines to
> some generic code - what you're describing here is definitely a problem
> and the idea was to provide a central place that has the logic and makes
> the decisions about what we should be doing.  If we don't have that it's
> going to get messy.
> 
> > I would rather have userspace be the single place getting notifications because
> > then we solve these details at a single location.
> 
> No argument on the place, making that place be userspace I'm less sold
> on.

fair enough. This would be an entire new entity, though, and users (chargers,
battery chips, USB controllers, USB PHYs, etc) should blindly notify this entity
and no care about what happened before or what are possible next states.

As long as we can guarantee that, then I'd be okay adding this to the kernel.

> > > Things more difficult, if nothing else it means we need to get whatever
> > > userspace component deployed in all relevant userspace stacks with
> > > appropriate configuration in order to do things.
> 
> > but that will be a thing for the kernel too. We will have to deploy this new
> > kernel component in all relevant stacks with appropriate configuration in order
> > to do things :-) No changes there.
> 
> Sure, but it's one project which is developed entirely in the open -
> that's a lot more manageable.

We can have a fully open source userland daemon too. Much like systemd, bluez,
neard, and many, many others. Why wouldn't that be a thing ?

> > > We do punt a lot of configuration to userspace for audio because there
> > > are substantial device specific policy elements in the configuration and
> > > it's a far from painless experience getting the code and configuration
> > > deployed where people need it, definitely a not great for users.
> 
> > it's the same for this situation. We will have a ton of device specific
> > configuration, specially with power delivery class, billboard class, the
> > alternate modes and so on. If we already do this part in userland, adding all
> > those extras will be, IMO, far simpler.
> 
> Can you elaborate a bit?  As far as I can tell all those are still in
> the generic USB space rather than the sort of device specifics I'm
> thinking of.

let's consider the billboard class, for example. With the new Type-C connector,
the extra CC pins get added and power delivery messaging goes on top of
that. Billboard class gives us the opportunity to send a power delivery request
to mux the data pins on the TypeC connector to something completely non-USB.

So we could run NATIVE PCIe (single lane though) on top of this. Native
DisplayPort, Native Thunderbolt (which the Fruit company announced will switch
to type-C, so that will be a thing), native VGA... the list goes on and on.

At that point, this is not entirely USB realm anymore :-)

Similar applies to power delivery charging profile themselves. Not all profiles
are mandatory. Some are optional and that will be completely device/board
specific. How an OEM implements that in the PCB is also completely
board-specific :-) Some might have a dumb IC hardcoding some messages, some
might have something largely more flexible.

> The things we've got with audio are a combination of tuning to taste and
> (even more importantly) very different ideas about what you want to do
> with an audio subsystem that influence how you set things up in a given
> use case.

the same thing will happen with Type-C and power delivery. There won't be tuning
to taste, of course :-) But there will be "very different ideas what what you
want to do with" your charging methodology.

> > > I think that where we can do it the bit where work out how the various
> > > components are connected and aggregate their functionality together is
> > > definitely a kernel job.  It means that userspace doesn't need to worry
> > > about implementation details of the particular system and instead gets a
> > > consistent, standard view of the device (in this case a USB port and how
> > > much power we can draw from it).
> 
> > Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> > charger/power_supply, a few regulators, a UDC view and this single userspace
> > daemon figures out how to tie those together.
> 
> That sounds worrying - it means that new hardware support needs
> supporting in every userspace (it seems inevitable that there will be at
> least some reimplementations because that's fun) which gets old really
> fast, and every userspace needs to understand every topology.

very true, but it's also true for a kernel solution.

It seems like you want it in the kernel because of it being convenient :-)

> > The view here isn't really a USB port, it's just a source of power. But how much
> > power we can draw from it depends on, possibly, a ton of different chips and
> > different notifications.
> 
> Right, yes - it's the power input/output associated with the USB port.
> The USB port is just the physical thing users can see so it's a good way
> to label it.  That could mean that we ought to move the aggregation bit
> into the power supply code of course, but then a lot of the decisions
> are going to be specific to USB.

in a sense, yes. But they can happen at a layer which knows nothing (or very
little) about USB. Here's an example:

Not everybody follows USB Battery Charging class. Some chargers don't short
D+/D- to signify they are a wall charger. Some will use different resistance
values on the ID pin to tell you how much current you can draw from them.

From a PMIC (or something else) point of view, all it needs is a tiny current
source and a an ADC, then it reports the ADC value to the upper layer. This
really doesn't even need to know that it's using an ID pin, or whatever.

> > > For the policy stuff we do want to have userspace be able to control
> > > things but we need to think about how to do that - for example does the
> > > entire flow need to be in userspace, or can we just expose settings
> > > which userspace can manage?
> 
> > considering the amount of different designs that are already out there and all
> > the extras that are going to show up due to type-c, I think we'd be better off
> > exposing as much to userspace as possible.
> 
> How different are the end goals of these designs really going to be
> though - that's more of the question for me?  Part of what the kernel is
> doing is hiding implementation details from userspace.  I'd expect
> userspace to be interested in things like the maximum current allowed in
> or out in a given mode rather than the details of how that is accomplished.

okay, this is a fair point :-) I just don't see, as of now at least, how we can
get to that in a way that's somewhat future-proof. It seems like we will
add more and more notifications until we can't maintain this anymore.

Say, we get a "certifiable" USB BC1.2 solution in the kernel. Current systems
are happy, we drop yet another out-of-tree set of patches from AOSP, every holds
hands and sings Kumbaya.

In another year or so, power delivery will be *the* thing. Now all the work done
assuming BC1.2 is gone, outdated. Adding power delivery brings an entire new set
of requirements. A new protocol stack (yes! it runs on CC pins, remember ?), new
messages, etc etc...

With Type-C there's no port type anymore. What we used to refer as Host/Hub
Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
type (let's call it type-c for short) which can provide profiles. These profiles
are negotiated using that new stack I pointed out above, not by checking
resistance on data lines or ID pin.

If we're not careful, it will be really difficult to make power delivery fit
into this.

Add to that the fact that the same power delivery pipe (the CC pins) can be used
to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
:-)

In short, whatever we do, needs to consider coping with incoming requests from
userspace at a minimum because userspace will need control over the port
alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
it'll be a pain to make a flexible/generic solution :-)

> > > I'm not sure that the NFC model is working well for everyone - it's OK
> > > if you happen to be running Android but if you aren't you're often left
> > > sitting there working out what to do with an Android HAL.  We can do the
> 
> > NFC works pretty well for neard, afaict. And without android.
> 
> Ah, that looks better than it was now - it looks like they're providing
> a reasonable hardware abstraction for messaging.  Still, there's other
> examples of this model that are less successful where the kernel
> interface isn't generic.
> 
> > The real difficulty here is coming up with an interface which we're agreeing to
> > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > to userland, we will have to support it.
> 
> To me that sounds like an argument for hiding things :)

the problem with hiding, though, is that once something new comes about, it
might not fit our current abstraction and it might be big PITA to support "old"
and new systems.

> > And this another reason I think a more granular approach is better, as it allows
> > us to easily add more pieces as they come along.
> 
> OTOH it's more work for the system as a whole.

it's more work outside the kernel, yes. The total amount of work (kernel +
userspace) will be the same, regardless if you hide it in the kernel or in userspace.

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:

> > > > Things more difficult, if nothing else it means we need to get whatever
> > > > userspace component deployed in all relevant userspace stacks with
> > > > appropriate configuration in order to do things.

> > > but that will be a thing for the kernel too. We will have to deploy this new
> > > kernel component in all relevant stacks with appropriate configuration in order
> > > to do things :-) No changes there.

> > Sure, but it's one project which is developed entirely in the open -
> > that's a lot more manageable.

> We can have a fully open source userland daemon too. Much like systemd, bluez,
> neard, and many, many others. Why wouldn't that be a thing ?

The trouble is getting traction on adoption.  Vendors have a habit of
doing things like finding problems and rather than reporting them
deciding that the open source solution isn't great and going and writing
their own thing (especially when they're in stealth mode) rather than
talking to anyone.  Sometimes we manage to avoid that but it can be
challenging, and often we only even hear about the new thing after it's
shipping and there's a lot of inertia behind changing it.  The kernel
ABIs tend to be a lot sticker than userspace things here.

> Similar applies to power delivery charging profile themselves. Not all profiles
> are mandatory. Some are optional and that will be completely device/board
> specific. How an OEM implements that in the PCB is also completely
> board-specific :-) Some might have a dumb IC hardcoding some messages, some
> might have something largely more flexible.

Right, and what I was trying to say was that it seems like the kernel
ought to be worrying about the board specific bits and userspace
worrying about what to do with those bits.

> > The things we've got with audio are a combination of tuning to taste and
> > (even more importantly) very different ideas about what you want to do
> > with an audio subsystem that influence how you set things up in a given
> > use case.

> the same thing will happen with Type-C and power delivery. There won't be tuning
> to taste, of course :-) But there will be "very different ideas what what you
> want to do with" your charging methodology.

Are those very different things about the use cases ("don't bother with
high speed data, get all the power you can" or whatever) or are they
about the details of the board?

> > > Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> > > charger/power_supply, a few regulators, a UDC view and this single userspace
> > > daemon figures out how to tie those together.

> > That sounds worrying - it means that new hardware support needs
> > supporting in every userspace (it seems inevitable that there will be at
> > least some reimplementations because that's fun) which gets old really
> > fast, and every userspace needs to understand every topology.

> very true, but it's also true for a kernel solution.

> It seems like you want it in the kernel because of it being convenient :-)

Yes, definitely - my experience both in terms of watching how people
like handset vendors often work internally and in terms of getting
things adopted is that it's a lot easier if you can have one component
you need to update to handle new hardware rather than multiple ones that
need to be upgraded for things that are purely board differences.

> > > The view here isn't really a USB port, it's just a source of power. But how much
> > > power we can draw from it depends on, possibly, a ton of different chips and
> > > different notifications.

> > Right, yes - it's the power input/output associated with the USB port.
> > The USB port is just the physical thing users can see so it's a good way
> > to label it.  That could mean that we ought to move the aggregation bit
> > into the power supply code of course, but then a lot of the decisions
> > are going to be specific to USB.

> in a sense, yes. But they can happen at a layer which knows nothing (or very
> little) about USB. Here's an example:

> Not everybody follows USB Battery Charging class. Some chargers don't short
> D+/D- to signify they are a wall charger. Some will use different resistance
> values on the ID pin to tell you how much current you can draw from them.

> From a PMIC (or something else) point of view, all it needs is a tiny current
> source and a an ADC, then it reports the ADC value to the upper layer. This
> really doesn't even need to know that it's using an ID pin, or whatever.

This doesn't seem much different to things like the various GPIO to
subsystem X drivers we've got - we already have some for IIO type
things IIRC.

> > How different are the end goals of these designs really going to be
> > though - that's more of the question for me?  Part of what the kernel is
> > doing is hiding implementation details from userspace.  I'd expect
> > userspace to be interested in things like the maximum current allowed in
> > or out in a given mode rather than the details of how that is accomplished.

> okay, this is a fair point :-) I just don't see, as of now at least, how we can
> get to that in a way that's somewhat future-proof. It seems like we will
> add more and more notifications until we can't maintain this anymore.

So we need to look at the new/upcoming USB specs and understand the
problem space, and how much of it we need to worry about right now in
this scope.

> With Type-C there's no port type anymore. What we used to refer as Host/Hub
> Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
> type (let's call it type-c for short) which can provide profiles. These profiles
> are negotiated using that new stack I pointed out above, not by checking
> resistance on data lines or ID pin.

Yup, which is a really cool feature and keeps us all employed too! :)
This is one reason for attaching things to the USB stack, right now it
does a limited negotiation but in future like you say there's more and
more features coming.

> Add to that the fact that the same power delivery pipe (the CC pins) can be used
> to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
> :-)

> In short, whatever we do, needs to consider coping with incoming requests from
> userspace at a minimum because userspace will need control over the port
> alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
> it'll be a pain to make a flexible/generic solution :-)

That seems to make sense to me - anything the kernel is able to do
autonomously for USB C should probably be pretty dumb.  Older USB
standards are already pretty dumb themselves (although inconsistently
standardised).

> > > The real difficulty here is coming up with an interface which we're agreeing to
> > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > > to userland, we will have to support it.

> > To me that sounds like an argument for hiding things :)

> the problem with hiding, though, is that once something new comes about, it
> might not fit our current abstraction and it might be big PITA to support "old"
> and new systems.

Does the problem not get easier if we break it down to just the charger
elements and realise that the USB C modes are going to have a lot more
policy in them?

> > > And this another reason I think a more granular approach is better, as it allows
> > > us to easily add more pieces as they come along.

> > OTOH it's more work for the system as a whole.

> it's more work outside the kernel, yes. The total amount of work (kernel +
> userspace) will be the same, regardless if you hide it in the kernel or in userspace.

Like I say I'm worried about deployment issues for the split solutions
making things harder than they should be.

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:
> On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote:
> > On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> > > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> 
> > > > > Things more difficult, if nothing else it means we need to get whatever
> > > > > userspace component deployed in all relevant userspace stacks with
> > > > > appropriate configuration in order to do things.
> 
> > > > but that will be a thing for the kernel too. We will have to deploy this new
> > > > kernel component in all relevant stacks with appropriate configuration in order
> > > > to do things :-) No changes there.
> 
> > > Sure, but it's one project which is developed entirely in the open -
> > > that's a lot more manageable.
> 
> > We can have a fully open source userland daemon too. Much like systemd, bluez,
> > neard, and many, many others. Why wouldn't that be a thing ?
> 
> The trouble is getting traction on adoption.  Vendors have a habit of doing
> things like finding problems and rather than reporting them deciding that the
> open source solution isn't great and going and writing their own thing
> (especially when they're in stealth mode) rather than talking to anyone.
> Sometimes we manage to avoid that but it can be challenging, and often we only
> even hear about the new thing after it's shipping and there's a lot of inertia
> behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> things here.

but this is not a solid enough argument to push anything into the kernel.

> > Similar applies to power delivery charging profile themselves. Not all
> > profiles are mandatory. Some are optional and that will be completely
> > device/board specific. How an OEM implements that in the PCB is also
> > completely board-specific :-) Some might have a dumb IC hardcoding some
> > messages, some might have something largely more flexible.
> 
> Right, and what I was trying to say was that it seems like the kernel ought to
> be worrying about the board specific bits and userspace worrying about what to
> do with those bits.

we're not saying anything different in this front. I, too, want kernel to deal
with certain details and expose notifications. Where we disagree is how granular
should these notifications be and where they should be sent to.

 > > The things we've got with audio are a combination of tuning to taste and
> > > (even more importantly) very different ideas about what you want to do
> > > with an audio subsystem that influence how you set things up in a given
> > > use case.
> 
> > the same thing will happen with Type-C and power delivery. There won't be tuning
> > to taste, of course :-) But there will be "very different ideas what what you
> > want to do with" your charging methodology.
> 
> Are those very different things about the use cases ("don't bother with
> high speed data, get all the power you can" or whatever) or are they
> about the details of the board?

I'm pretty sure there will be interesting designs in the market. I'm pretty sure
there will be type-C systems which won't support USB 3.1 for example, even
though they'll support USB Power Delivery in its entirety.

> > > > Actually, I was thinking about it in a more granular fashion. Kernel
> > > > exposes a charger/power_supply, a few regulators, a UDC view and this
> > > > single userspace daemon figures out how to tie those together.
> 
> > > That sounds worrying - it means that new hardware support needs
> > > supporting in every userspace (it seems inevitable that there will be at
> > > least some reimplementations because that's fun) which gets old really
> > > fast, and every userspace needs to understand every topology.
> 
> > very true, but it's also true for a kernel solution.
> 
> > It seems like you want it in the kernel because of it being convenient :-)
> 
> Yes, definitely - my experience both in terms of watching how people
> like handset vendors often work internally and in terms of getting
> things adopted is that it's a lot easier if you can have one component
> you need to update to handle new hardware rather than multiple ones that
> need to be upgraded for things that are purely board differences.

IMO, just because you have it in the kernel, it doesn't mean it'll be
adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
used by Android (or has that changed ?)

> > > > The view here isn't really a USB port, it's just a source of power. But how much
> > > > power we can draw from it depends on, possibly, a ton of different chips and
> > > > different notifications.
> 
> > > Right, yes - it's the power input/output associated with the USB port.
> > > The USB port is just the physical thing users can see so it's a good way
> > > to label it.  That could mean that we ought to move the aggregation bit
> > > into the power supply code of course, but then a lot of the decisions
> > > are going to be specific to USB.
> 
> > in a sense, yes. But they can happen at a layer which knows nothing (or very
> > little) about USB. Here's an example:
> 
> > Not everybody follows USB Battery Charging class. Some chargers don't short
> > D+/D- to signify they are a wall charger. Some will use different resistance
> > values on the ID pin to tell you how much current you can draw from them.
> 
> > From a PMIC (or something else) point of view, all it needs is a tiny current
> > source and a an ADC, then it reports the ADC value to the upper layer. This
> > really doesn't even need to know that it's using an ID pin, or whatever.
> 
> This doesn't seem much different to things like the various GPIO to
> subsystem X drivers we've got - we already have some for IIO type
> things IIRC.

that's really not what I was trying to point out :-)

> > > How different are the end goals of these designs really going to be
> > > though - that's more of the question for me?  Part of what the kernel is
> > > doing is hiding implementation details from userspace.  I'd expect
> > > userspace to be interested in things like the maximum current allowed in
> > > or out in a given mode rather than the details of how that is accomplished.
> 
> > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > get to that in a way that's somewhat future-proof. It seems like we will
> > add more and more notifications until we can't maintain this anymore.
> 
> So we need to look at the new/upcoming USB specs and understand the

not upcoming, it's already out there.

> problem space, and how much of it we need to worry about right now in
> this scope.
> 
> > With Type-C there's no port type anymore. What we used to refer as Host/Hub
> > Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
> > type (let's call it type-c for short) which can provide profiles. These profiles
> > are negotiated using that new stack I pointed out above, not by checking
> > resistance on data lines or ID pin.
> 
> Yup, which is a really cool feature and keeps us all employed too! :)
> This is one reason for attaching things to the USB stack, right now it
> does a limited negotiation but in future like you say there's more and
> more features coming.

keep in mind that the same stack used to change a charging current, will also be
used to tell the other end to mux those lines differently.

> > Add to that the fact that the same power delivery pipe (the CC pins) can be used
> > to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
> > :-)
> 
> > In short, whatever we do, needs to consider coping with incoming requests from
> > userspace at a minimum because userspace will need control over the port
> > alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
> > it'll be a pain to make a flexible/generic solution :-)
> 
> That seems to make sense to me - anything the kernel is able to do
> autonomously for USB C should probably be pretty dumb.  Older USB
> standards are already pretty dumb themselves (although inconsistently
> standardised).

right.

> > > > The real difficulty here is coming up with an interface which we're agreeing to
> > > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > > > to userland, we will have to support it.
> 
> > > To me that sounds like an argument for hiding things :)
> 
> > the problem with hiding, though, is that once something new comes about, it
> > might not fit our current abstraction and it might be big PITA to support "old"
> > and new systems.
> 
> Does the problem not get easier if we break it down to just the charger
> elements and realise that the USB C modes are going to have a lot more
> policy in them?

the thing with USB C is that it's not only for negotiating a charging power
(with power delivery you're not necessarily tied to 5V, btw), that very stack
will also be used to change to other alternate modes, and those can be anything:
Thunderbolt, PCIe, DisplayPort, etc.

> > > > And this another reason I think a more granular approach is better, as it allows
> > > > us to easily add more pieces as they come along.
> 
> > > OTOH it's more work for the system as a whole.
> 
> > it's more work outside the kernel, yes. The total amount of work (kernel +
> > userspace) will be the same, regardless if you hide it in the kernel or in userspace.
> 
> Like I say I'm worried about deployment issues for the split solutions
> making things harder than they should be.

and I'm worried about us having too much inside the kernel and that making
things harder than they should be :-)

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote:
> On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:

> > The trouble is getting traction on adoption.  Vendors have a habit of doing
> > things like finding problems and rather than reporting them deciding that the
> > open source solution isn't great and going and writing their own thing
> > (especially when they're in stealth mode) rather than talking to anyone.
> > Sometimes we manage to avoid that but it can be challenging, and often we only
> > even hear about the new thing after it's shipping and there's a lot of inertia
> > behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> > things here.

> but this is not a solid enough argument to push anything into the kernel.

To my mind it is a concern when considering design - it's not the only
consideration certainly but it should influence if or how we split
things.

> > > the same thing will happen with Type-C and power delivery. There won't be tuning
> > > to taste, of course :-) But there will be "very different ideas what what you
> > > want to do with" your charging methodology.

> > Are those very different things about the use cases ("don't bother with
> > high speed data, get all the power you can" or whatever) or are they
> > about the details of the board?

> I'm pretty sure there will be interesting designs in the market. I'm pretty sure
> there will be type-C systems which won't support USB 3.1 for example, even
> though they'll support USB Power Delivery in its entirety.

That's sounding like generic USB stuff to me.

> > Yes, definitely - my experience both in terms of watching how people
> > like handset vendors often work internally and in terms of getting
> > things adopted is that it's a lot easier if you can have one component
> > you need to update to handle new hardware rather than multiple ones that
> > need to be upgraded for things that are purely board differences.

> IMO, just because you have it in the kernel, it doesn't mean it'll be
> adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
> used by Android (or has that changed ?)

That's always been a bit of a myth - most Android systems use runtime PM
extensively when they're running, the discussion is really about the
edge case where you're idling the system.  The Android suspend behaviour
is about the system idle case and is as much about providing a simple
way to go into an idle state, especially bearing in mind that they have
apps installed from the app store there, as anything else.  It's the
making sure things are idle part of things that's different.

> > > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > > get to that in a way that's somewhat future-proof. It seems like we will
> > > add more and more notifications until we can't maintain this anymore.

> > So we need to look at the new/upcoming USB specs and understand the

> not upcoming, it's already out there.

I assume there's ongoing work on further revisions to the spec though?

> > Yup, which is a really cool feature and keeps us all employed too! :)
> > This is one reason for attaching things to the USB stack, right now it
> > does a limited negotiation but in future like you say there's more and
> > more features coming.

> keep in mind that the same stack used to change a charging current, will also be
> used to tell the other end to mux those lines differently.

Sure.

> > Does the problem not get easier if we break it down to just the charger
> > elements and realise that the USB C modes are going to have a lot more
> > policy in them?

> the thing with USB C is that it's not only for negotiating a charging power
> (with power delivery you're not necessarily tied to 5V, btw), that very stack
> will also be used to change to other alternate modes, and those can be anything:
> Thunderbolt, PCIe, DisplayPort, etc.

Surely these features have sufficient orthogonality to allow us to split
things up and deal with some of the problems while providing spaces for
future work?

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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

Hi,

On Mon, Oct 05, 2015 at 05:18:33PM +0100, Mark Brown wrote:
> On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote:
> > On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:
> 
> > > The trouble is getting traction on adoption.  Vendors have a habit of doing
> > > things like finding problems and rather than reporting them deciding that the
> > > open source solution isn't great and going and writing their own thing
> > > (especially when they're in stealth mode) rather than talking to anyone.
> > > Sometimes we manage to avoid that but it can be challenging, and often we only
> > > even hear about the new thing after it's shipping and there's a lot of inertia
> > > behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> > > things here.
> 
> > but this is not a solid enough argument to push anything into the kernel.
> 
> To my mind it is a concern when considering design - it's not the only
> consideration certainly but it should influence if or how we split
> things.

sure

> > > > the same thing will happen with Type-C and power delivery. There won't be tuning
> > > > to taste, of course :-) But there will be "very different ideas what what you
> > > > want to do with" your charging methodology.
> 
> > > Are those very different things about the use cases ("don't bother with
> > > high speed data, get all the power you can" or whatever) or are they
> > > about the details of the board?
> 
> > I'm pretty sure there will be interesting designs in the market. I'm pretty sure
> > there will be type-C systems which won't support USB 3.1 for example, even
> > though they'll support USB Power Delivery in its entirety.
> 
> That's sounding like generic USB stuff to me.
> 
> > > Yes, definitely - my experience both in terms of watching how people
> > > like handset vendors often work internally and in terms of getting
> > > things adopted is that it's a lot easier if you can have one component
> > > you need to update to handle new hardware rather than multiple ones that
> > > need to be upgraded for things that are purely board differences.
> 
> > IMO, just because you have it in the kernel, it doesn't mean it'll be
> > adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
> > used by Android (or has that changed ?)
> 
> That's always been a bit of a myth - most Android systems use runtime PM
> extensively when they're running, the discussion is really about the
> edge case where you're idling the system.  The Android suspend behaviour
> is about the system idle case and is as much about providing a simple
> way to go into an idle state, especially bearing in mind that they have
> apps installed from the app store there, as anything else.  It's the
> making sure things are idle part of things that's different.

okay

> > > > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > > > get to that in a way that's somewhat future-proof. It seems like we will
> > > > add more and more notifications until we can't maintain this anymore.
> 
> > > So we need to look at the new/upcoming USB specs and understand the
> 
> > not upcoming, it's already out there.
> 
> I assume there's ongoing work on further revisions to the spec though?

that'd be a correct assumption

> > > Does the problem not get easier if we break it down to just the charger
> > > elements and realise that the USB C modes are going to have a lot more
> > > policy in them?
> 
> > the thing with USB C is that it's not only for negotiating a charging power
> > (with power delivery you're not necessarily tied to 5V, btw), that very stack
> > will also be used to change to other alternate modes, and those can be anything:
> > Thunderbolt, PCIe, DisplayPort, etc.
> 
> Surely these features have sufficient orthogonality to allow us to split
> things up and deal with some of the problems while providing spaces for
> future work?

yeah, might. As long as we keep that voice in our heads that things are likely
to change.

-- 
balbi

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

Re: [Device-mainlining] [PATCH v4 1/5] gadget: Introduce the notifier functions

[Bjorn Andersson]

On Fri, Oct 2, 2015 at 10:23 AM, Felipe Balbi via device-mainlining
<device-mainlining@lists.linuxfoundation.org> wrote:
[..]
> The real difficulty here is coming up with an interface which we're agreeing to
> supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> to userland, we will have to support it.
>

Isn't this the main reason for not creating a user space ABI now?

Running with the in-kernel hooks would allow us to get things working
now, we can change it as we wish, we can explore the difficulties and
we can create an ABI from that - if we need to - that might have a
chance to work for the next 30 years.

Regards,
Bjorn

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Pavel Machek]

HI!

> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> 
> Greg, this is the kind of thing I wanted to avoid adding more of.
> 
> I was wondering if you would accept subsystems using kdbus for
> this sort of notification. I'm okay waiting for kdbus for another
> couple merge windows (if we have to) before that's merged, but
> if we take this raw notifier approach now, we will end up having
> to support it forever.
> 
> Also, because soon enough we will have to support USB Power Delivery
> with Type C connector, this is bound to change in the coming months.
> 
> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).
> 
> How would you feel about that ?

So init=/bin/bash boot no longer provides machine that charges itself?

That would be bad. Traditionally, hardware controls battery charging,
and if hardware needs some help, we should do it in kernel, to mask
the difference from userspace.
								Pavel

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Pavel Machek]

Hi!

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.
> 
> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use
> some mostly discrete setup and so on...
> 
> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).
> 
> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the other
> end.

N900 does charging from kernel these days.

Not being able to boot generic distribution would be regression there...

										Pavel

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Felipe Balbi]

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


Hi,

Pavel Machek <pavel@ucw.cz> writes:
> HI!
>
>> > +	int ret;
>> > +
>> > +	mutex_lock(&gadget->lock);
>> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
>> 
>> Greg, this is the kind of thing I wanted to avoid adding more of.
>> 
>> I was wondering if you would accept subsystems using kdbus for
>> this sort of notification. I'm okay waiting for kdbus for another
>> couple merge windows (if we have to) before that's merged, but
>> if we take this raw notifier approach now, we will end up having
>> to support it forever.
>> 
>> Also, because soon enough we will have to support USB Power Delivery
>> with Type C connector, this is bound to change in the coming months.
>> 
>> Frankly, I wanted all of this to be decided in userland with the
>> kernel just providing notification and basic safety checks (we don't
>> want to allow a bogus userspace daemon frying anybody's devices).
>> 
>> How would you feel about that ?
>
> So init=/bin/bash boot no longer provides machine that charges itself?
>
> That would be bad. Traditionally, hardware controls battery charging,
> and if hardware needs some help, we should do it in kernel, to mask
> the difference from userspace.

this is a very valid point which I hadn't considered :-)

Seems like kernel it is, no matter how easy or how difficult it gets.

Mark, when can we try to have a discussion about how to get this
upstream ? It seems like designing everything in the mailing list will
just take forever. Any ideas ?

-- 
balbi

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

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

On Fri, Oct 09, 2015 at 04:17:27PM -0500, Felipe Balbi wrote:

> Mark, when can we try to have a discussion about how to get this
> upstream ? It seems like designing everything in the mailing list will
> just take forever. Any ideas ?

Can we take this off-list?  I'm in the UK, Baolin is in China and I
believe you're in the US so it might be a fun one to schedule...

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

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Pavel Machek]

Hi!

> >> Also, because soon enough we will have to support USB Power Delivery
> >> with Type C connector, this is bound to change in the coming months.
> >> 
> >> Frankly, I wanted all of this to be decided in userland with the
> >> kernel just providing notification and basic safety checks (we don't
> >> want to allow a bogus userspace daemon frying anybody's devices).
> >> 
> >> How would you feel about that ?
> >
> > So init=/bin/bash boot no longer provides machine that charges itself?
> >
> > That would be bad. Traditionally, hardware controls battery charging,
> > and if hardware needs some help, we should do it in kernel, to mask
> > the difference from userspace.
> 
> this is a very valid point which I hadn't considered :-)
> 
> Seems like kernel it is, no matter how easy or how difficult it gets.

Basically yes. Note that for some bells & whistles, you can require userland help.

For example it would probably be ok if it charged slower than usual... but I guess
that does not really help in this case.

Best regards,
									Pavel

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

[PATCH v4 1/5] gadget: Introduce the notifier functions

[Baolin Wang]

The usb charger framework is based on usb gadget. The usb charger
need to be notified the state changing of usb gadget to confirm the
usb charger state.

Thus this patch adds a notifier mechanism for usb gadget to report a
event to usb charger when the usb gadget state is changed.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..4238fc3 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_register(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* ------------------------------------------------------------------------- */
 
 /**
@@ -226,6 +252,10 @@ static void usb_gadget_state_work(struct work_struct *work)
 	struct usb_gadget *gadget = work_to_gadget(work);
 	struct usb_udc *udc = gadget->udc;
 
+	mutex_lock(&gadget->lock);
+	raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
+	mutex_unlock(&gadget->lock);
+
 	if (udc)
 		sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -364,6 +394,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	dev_set_name(&gadget->dev, "gadget");
 	INIT_WORK(&gadget->work, usb_gadget_state_work);
+	RAW_INIT_NOTIFIER_HEAD(&gadget->nh);
+	mutex_init(&gadget->lock);
 	gadget->dev.parent = parent;
 
 #ifdef	CONFIG_HAS_DMA
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c14a69b..755e8bc 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -609,6 +609,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	struct raw_notifier_head	nh;
+	struct mutex			lock;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -1183,6 +1185,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget *gadget,
 
 /*-------------------------------------------------------------------------*/
 
+/**
+ * Register a notifiee to get notified by any attach status changes from
+ * the usb gadget
+ */
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
+
+/* Unregister a notifiee from the usb gadget */
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
 /* utility to set gadget state properly */
 
 extern void usb_gadget_set_state(struct usb_gadget *gadget,
-- 
1.7.9.5