Introduce usb charger framework to deal with the usb gadget power negotation

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.

Quoted 4 lines · 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.

Quoted 4 lines

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

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

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

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

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

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

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

Signed-off-by Baolin Wang <[email protected]>

---
 drivers/usb/gadget/Kconfig      |    7 +
 drivers/usb/gadget/Makefile     |    1 +
 drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++

drivers/usb/gadget/Kconfig +0 -0

index af5d922..82a5b3c 100644

drivers/usb/gadget/Makefile +1 -0

index 598a67d..1e421c1 100644
@@ -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

drivers/usb/gadget/charger.c new file +10 -0

new file mode 100644
index 0000000..82a9973
@@ -0,0 +1,669 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+

drivers/usb/gadget/charger.c new file +22 -0

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

drivers/usb/gadget/charger.c new file +18 -0

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

drivers/usb/gadget/charger.c new file +18 -0

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

drivers/usb/gadget/charger.c new file +18 -0

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

drivers/usb/gadget/charger.c new file +56 -0

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

drivers/usb/gadget/charger.c new file +245 -0

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

drivers/usb/gadget/charger.c new file +7 -0

+		return -EINVAL;
+
+	device_unregister(&uchger->dev);
+	return 0;
+}
+
+static void devm_uchger_dev_unreg(struct device *dev, void *res)

drivers/usb/gadget/charger.c new file +50 -0

+		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;

drivers/usb/gadget/charger.c new file +56 -0

+		return -EINVAL;
+
+	uchger = kzalloc(sizeof(struct usb_charger), GFP_KERNEL);
+	if (!uchger)
+		return -ENOMEM;
+
+	uchger->type = UNKNOWN_TYPE;
+	uchger->state = USB_CHARGER_DEFAULT;
+	uchger->id = -1;
+	uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
+	uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
+	uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
+	uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
+	uchger->get_charger_type = NULL;
+
+	mutex_init(&uchger->lock);
+	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
+
+	/* register a notifier on a extcon device if it is exsited */
+	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
+	if (!IS_ERR_OR_NULL(edev)) {
+		uchger->extcon_dev = edev;
+		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
+		uchger->extcon_nb.uchger = uchger;
+		extcon_register_notifier(edev, EXTCON_USB,
+					 &uchger->extcon_nb.nb);
+	}
+
+	/* to check if the usb charger is link to a power supply */
+	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
+					       "power-supplies");
+	if (!IS_ERR_OR_NULL(psy))
+		uchger->psy = psy;
+	else
+		uchger->psy = NULL;
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = ugadget->state;
+
+	/* register a new usb charger */
+	ret = usb_charger_register(&ugadget->dev, uchger);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	kfree(uchger);
+	return ret;
+}
+

include/linux/usb/usb_charger.h new file +15 -0

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

drivers/usb/gadget/charger.c line 20

Quoted 4 lines · Baolin Wang

drivers/usb/gadget/charger.c new file +4 -0

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

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

Quoted 1 line

+static DEVICE_ATTR_RW(sdp_limit);

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

[ text/plain ]

Quoted 9 lines · Felipe Balbi

drivers/usb/gadget/charger.c line 20

Quoted 4 lines · Baolin Wang

drivers/usb/gadget/charger.c new file +4 -0

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

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

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

Quoted 4 lines

Quoted 1 line

+static DEVICE_ATTR_RW(sdp_limit);

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

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

Quoted 4 lines

---
 drivers/usb/gadget/Kconfig      |    7 +
 drivers/usb/gadget/Makefile     |    1 +
 drivers/usb/gadget/charger.c    |  669 +++++++++++++++++++++++++++++++++++++++

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

Quoted 9 lines

drivers/usb/gadget/Kconfig +0 -0

index af5d922..82a5b3c 100644
@@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
      help
         It supports the serial gadget can be used as a console.
 

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 141

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+config USB_CHARGER
+     bool "USB charger support"
+     help
+       The usb charger driver based on the usb gadget that makes an
+       enhancement to a power driver which can set the current limitation
+       when the usb charger is added or removed.

sure you don't depend on anything ?

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 143

Quoted 3 lines

drivers/usb/gadget/Kconfig +2 -0

+
+#define DEFAULT_CUR_PROTECT  (50)

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

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 144

Quoted 2 lines

drivers/usb/gadget/Kconfig +1 -0

+#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)

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

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE

Quoted 5 lines

drivers/usb/gadget/Kconfig +4 -0

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

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

drivers/usb/gadget/Kconfig

Quoted 8 lines

drivers/usb/gadget/Kconfig +7 -0

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

nit-picking but I'd rather call this struct device *dev. Also, I'm not

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

Quoted 28 lines

drivers/usb/gadget/Kconfig +27 -0

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

drivers/usb/gadget/Kconfig

Quoted 5 lines

drivers/usb/gadget/Kconfig +5 -0

+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(sdp_limit);

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

Quoted 23 lines

drivers/usb/gadget/Kconfig +22 -0

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

drivers/usb/gadget/Kconfig

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+     if (ret < 0)
+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(dcp_limit);

likewise, why RW ? Missing doc.

Quoted 23 lines

drivers/usb/gadget/Kconfig +22 -0

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

drivers/usb/gadget/Kconfig

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+     if (ret < 0)
+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(cdp_limit);

why RW? Where's doc ?

Quoted 23 lines

drivers/usb/gadget/Kconfig +22 -0

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

drivers/usb/gadget/Kconfig

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+     if (ret < 0)
+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(aca_limit);

why RW ? where's doc ?

Quoted 2 lines

+static struct attribute *usb_charger_attrs[] = {

const ?

drivers/usb/gadget/Kconfig

Quoted 8 lines

drivers/usb/gadget/Kconfig +7 -0

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

static ?

drivers/usb/gadget/Kconfig

Quoted 7 lines

drivers/usb/gadget/Kconfig +6 -0

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

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

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

drivers/usb/gadget/Kconfig

Quoted 4 lines

drivers/usb/gadget/Kconfig +3 -0

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

still not convinced this deserves to be a bus_type.

drivers/usb/gadget/Kconfig

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

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

WARN() ?

drivers/usb/gadget/Kconfig

Quoted 3 lines

drivers/usb/gadget/Kconfig +3 -0

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

WARN()

Quoted 12 lines

drivers/usb/gadget/Kconfig +11 -0

+             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);
+}
+

drivers/usb/gadget/Kconfig

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+void devm_usb_charger_unregister(struct device *dev,
+                              struct usb_charger *uchger)
+{
+     devres_release(dev, devm_uchger_dev_unreg,
+                    devm_uchger_dev_match, uchger);
+}

does this need EXPORT_SYMBOL_GPL() too ?

drivers/usb/gadget/Kconfig

Quoted 13 lines

drivers/usb/gadget/Kconfig +12 -0

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

WARN()

drivers/usb/gadget/Kconfig

Quoted 9 lines

drivers/usb/gadget/Kconfig +8 -0

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

WARN()

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

drivers/usb/gadget/Kconfig

Quoted 7 lines

drivers/usb/gadget/Kconfig +6 -0

+
+static int __init usb_charger_sysfs_init(void)
+{
+     return subsys_system_register(&usb_charger_subsys, NULL);
+}
+core_initcall(usb_charger_sysfs_init);

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

drivers/usb/gadget/Kconfig

Quoted 4 lines

drivers/usb/gadget/Kconfig +3 -0

+MODULE_AUTHOR("Baolin Wang <[email protected]>");
+MODULE_DESCRIPTION("USB charger driver");
+MODULE_LICENSE("GPL");

GPLv2 or GPLv2+ ??

include/linux/usb/usb_charger.h

Quoted 6 lines

include/linux/usb/usb_charger.h new file +0 -0

new file mode 100644
index 0000000..eed422f

usb_ is redundant. I'd prefer to:

#include <linux/usb/charger.h>

include/linux/usb/usb_charger.h line 25

Quoted 8 lines

include/linux/usb/usb_charger.h new file +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

--
balbi

Quoted 8 lines

Quoted 8 lines

drivers/usb/gadget/Kconfig +0 -0

index af5d922..82a5b3c 100644
@@ -133,6 +133,13 @@ config U_SERIAL_CONSOLE
      help
         It supports the serial gadget can be used as a console.
 

Quoted 8 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 141

Quoted 6 lines

drivers/usb/gadget/Kconfig +6 -0

+config USB_CHARGER
+     bool "USB charger support"
+     help
+       The usb charger driver based on the usb gadget that makes an
+       enhancement to a power driver which can set the current limitation
+       when the usb charger is added or removed.

sure you don't depend on anything ?

It just depends on USB_GADGET.

Quoted 4 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 143

Quoted 2 lines

drivers/usb/gadget/Kconfig +2 -0

+
+#define DEFAULT_CUR_PROTECT  (50)

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

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

Quoted 6 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 144

Quoted 1 line

drivers/usb/gadget/Kconfig +1 -0

+#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)

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

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

Quoted 7 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE

Quoted 4 lines

drivers/usb/gadget/Kconfig +4 -0

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

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

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

Quoted 9 lines

drivers/usb/gadget/Kconfig

Quoted 7 lines

drivers/usb/gadget/Kconfig +7 -0

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

nit-picking but I'd rather call this struct device *dev. Also, I'm not

OK.

Quoted 2 lines

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

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

Quoted 27 lines

Quoted 27 lines

drivers/usb/gadget/Kconfig +27 -0

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

Quoted 8 lines

drivers/usb/gadget/Kconfig

Quoted 5 lines

drivers/usb/gadget/Kconfig +5 -0

+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(sdp_limit);

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

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

Quoted 9 lines

drivers/usb/gadget/Kconfig

Quoted 7 lines

drivers/usb/gadget/Kconfig +7 -0

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

static ?

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

Quoted 9 lines

drivers/usb/gadget/Kconfig

Quoted 7 lines

drivers/usb/gadget/Kconfig +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

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

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

+#define DEFAULT_CUR_PROTECT (50)

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

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

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

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

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

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

Quoted 9 lines

Quoted 8 lines

drivers/usb/gadget/charger.c line 33

Quoted 5 lines

drivers/usb/gadget/charger.c new file +4 -0

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

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

Quoted 2 lines

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

and ARRAY_SIZE(arr) is not enough ?

Quoted 6 lines

Quoted 3 lines

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

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

I guess a class would fit better in this case.

Quoted 37 lines

Quoted 28 lines

Quoted 28 lines

drivers/usb/gadget/charger.c new file +27 -0

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

Quoted 8 lines

drivers/usb/gadget/charger.c line 73

Quoted 5 lines

drivers/usb/gadget/charger.c new file +5 -0

+             return ret;
+
+     return count;
+}
+static DEVICE_ATTR_RW(sdp_limit);

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

Quoted 3 lines

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

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

a) you are connected to a dedicated charger

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

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

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

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

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

you *know* 500mA is okay, but you _can_ get more (it is a charging port after all). So charging IC will connect a 500mA load and step upwards until VBUS drops a little, just like (a) above.

This means you will always charger with maximum rating for this host charging port.

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

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

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

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

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

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

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

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

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

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

Just trust the charging IC to do a good job.

Quoted 10 lines

Quoted 9 lines

include/linux/usb/usb_charger.h line 25

Quoted 7 lines

include/linux/usb/usb_charger.h new file +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

Quoted 2 lines

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

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

--
balbi

Quoted 14 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

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

Quoted 2 lines

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

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

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

include/linux/usb/usb_charger.h line 25

Quoted 7 lines

include/linux/usb/usb_charger.h new file +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

Quoted 5 lines

Quoted 2 lines

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

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

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

Hi,

Quoted 14 lines · Baolin Wang

Quoted 14 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

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

Quoted 6 lines

Quoted 2 lines

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

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

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

IMO ;-)

Quoted 16 lines

Quoted 15 lines

Quoted 10 lines

Quoted 9 lines

include/linux/usb/usb_charger.h line 25

Quoted 7 lines

include/linux/usb/usb_charger.h new file +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

Quoted 2 lines

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

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

Quoted 2 lines

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

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

--
balbi

[ text/plain ]

Quoted 21 lines · Felipe Balbi

Hi,

Quoted 17 lines · Baolin Wang

Quoted 16 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

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

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

Quoted 5 lines

Quoted 3 lines

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

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

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

Quoted 19 lines

Quoted 18 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

Quoted 5 lines

Quoted 2 lines

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

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

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

Quoted 4 lines

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

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

Quoted 27 lines · Baolin Wang

[ text/plain ]

Quoted 25 lines · Felipe Balbi

Hi,

Quoted 20 lines · Baolin Wang

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

Quoted 6 lines

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

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

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

Quoted 3 lines

Quoted 1 line

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

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

int default_current_limit = 500;

if (gadget->speed >= USB_SPEED_SUPER)

        default_current_limit = 900;

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +7 -0

+
+/* USB charger state */
+enum usb_charger_state {
+     USB_CHARGER_DEFAULT,
+     USB_CHARGER_PRESENT,
+     USB_CHARGER_REMOVE,
+};

userland really doesn't need these two ?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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

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

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

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

Quoted 3 lines

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

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

--
balbi

Quoted 3 lines

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

Quoted 19 lines

Quoted 6 lines

Quoted 2 lines

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

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

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

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

Quoted 11 lines

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

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

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

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

Hi!

Quoted 3 lines

Quoted 3 lines

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

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

Quoted 19 lines

Quoted 19 lines

Quoted 6 lines

Quoted 2 lines

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

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

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

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

Quoted 4 lines

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

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

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

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

Quoted 6 lines

Quoted 6 lines

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

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

Quoted 5 lines

Quoted 5 lines

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

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

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

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

Quoted 14 lines

Quoted 9 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

Quoted 4 lines

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

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

Quoted 7 lines

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

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

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

Quoted 12 lines

Quoted 11 lines

Quoted 11 lines

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

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

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

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

Quoted 2 lines

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

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

Quoted 2 lines

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

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

Quoted 22 lines

Quoted 14 lines

Quoted 9 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

Quoted 4 lines

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

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

Quoted 7 lines

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

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

Quoted 6 lines

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

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

Quoted 15 lines

Quoted 14 lines

Quoted 11 lines

Quoted 11 lines

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

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

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

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

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

Quoted 3 lines

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

Not more unsafe than loading wall chargers with "lets see how much we can get out of it" algorithm. Plus actually required to charge your

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

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

Quoted 7 lines

Quoted 2 lines

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

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

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

Quoted 7 lines

Quoted 6 lines

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

Quoted 3 lines

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

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

Quoted 6 lines

Quoted 3 lines

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

Not more unsafe than loading wall chargers with "lets see how much we can get out of it" algorithm. Plus actually required to charge your

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

Quoted 2 lines

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

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

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

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

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

Quoted 3 lines

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

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

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 3 lines

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

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

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

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

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

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

Not more unsafe than loading wall chargers with "lets see how much we can get out of it" algorithm. Plus actually required to charge your

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

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

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

Quoted 5 lines

Quoted 2 lines

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

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

Manual control of n900 charging is done by:

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

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 2 lines

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

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

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

Quoted 3 lines

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

They already can go over the limit, for example using cable linked above. I have several such cables here. I also have various wall

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

Quoted 6 lines

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

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

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

Quoted 15 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

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

Not more unsafe than loading wall chargers with "lets see how much we can get out of it" algorithm. Plus actually required to charge your

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

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

Quoted 1 line

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

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

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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

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

Quoted 3 lines

Manual control of n900 charging is done by:

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

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

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

Quoted 2 lines

They already can go over the limit, for example using cable linked above. I have several such cables here. I also have various wall

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

Quoted 3 lines

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

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

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

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

Quoted 10 lines

Quoted 6 lines

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

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

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

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

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

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

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

Not more unsafe than loading wall chargers with "lets see how much we can get out of it" algorithm. Plus actually required to charge your

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

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

Quoted 5 lines

Quoted 1 line

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

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

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

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

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

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

Manual control of n900 charging is done by:

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

Quoted 3 lines

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

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

Quoted 5 lines

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

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

Quoted 5 lines

Quoted 2 lines

They already can go over the limit, for example using cable linked above. I have several such cables here. I also have various wall

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

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

Quoted 8 lines

Quoted 3 lines

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

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

Quoted 5 lines

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

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

Quoted 5 lines

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

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

Quoted 2 lines · Baolin Wang

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

I guess a class would fit better in this case.

Quoted 6 lines · Felipe Balbi

Quoted 2 lines · Baolin Wang

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

I guess a class would fit better in this case.

IIRC Greg didn't want new classes?

Quoted 1 line · Mark Brown

IIRC Greg didn't want new classes?

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

Quoted 5 lines · Felipe Balbi

Quoted 1 line · Mark Brown

IIRC Greg didn't want new classes?

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

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

Quoted 3 lines · Mark Brown

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

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

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

Quoted 8 lines · Felipe Balbi

Quoted 3 lines · Mark Brown

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

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

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

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

Quoted 2 lines

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

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

a) you are connected to a dedicated charger

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

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

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

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

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

Quoted 6 lines

Quoted 2 lines

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

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

root should be allowed to do that.

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

N900 will simply not charge from .5A.

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

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

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

+#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT)

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

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

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

Quoted 1 line

root should be allowed to do that.

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

Quoted 1 line

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

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

Quoted 1 line

N900 will simply not charge from .5A.

it used to with original maemo userspace/kernel.

Quoted 14 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

Quoted 6 lines

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

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

Quoted 4 lines

Quoted 1 line

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

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

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

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

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

Quoted 3 lines

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

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

Hi,

Quoted 5 lines · Pavel Machek

Quoted 4 lines

Quoted 1 line

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

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

Quoted 2 lines

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

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

Quoted 25 lines

Quoted 24 lines

Quoted 14 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

Quoted 6 lines

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

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

Quoted 2 lines

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

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

Quoted 12 lines · Felipe Balbi

Hi,

Quoted 7 lines · Pavel Machek

Quoted 4 lines

Quoted 1 line

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

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

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

Quoted 4 lines

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

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

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

Quoted 28 lines

Quoted 27 lines

Quoted 21 lines

Quoted 14 lines

Quoted 9 lines

a) you are connected to a dedicated charger

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

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

Quoted 4 lines

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

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

Quoted 6 lines

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

Quoted 3 lines

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

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

Quoted 3 lines

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

But cellphone user knows what he connected his charger to, and that's why it is useful to be able to lower the current. Even when you said "less is just stupid" I demonstrated it is not, at least in case when your power source is a battery.

Quoted 3 lines

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

Hi,

Quoted 3 lines · Felipe Balbi

Quoted 3 lines

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

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

Quoted 11 lines · Felipe Balbi

Hi,

Quoted 3 lines · Felipe Balbi

Quoted 3 lines

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

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

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

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

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

Hi,

Quoted 20 lines · Pavel Machek

Quoted 11 lines · Felipe Balbi

Hi,

Quoted 3 lines · Felipe Balbi

Quoted 3 lines

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

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

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

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

Quoted 2 lines

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

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

Quoted 21 lines

Quoted 20 lines · Pavel Machek

Quoted 11 lines · Felipe Balbi

Hi,

Quoted 3 lines · Felipe Balbi

Quoted 3 lines

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

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

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

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

Quoted 6 lines

Quoted 2 lines

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

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

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

Even after I described the example with device running off power bank on a train? Would you explain your reasoning?

Sent: 18 April 2016 11:40

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 6 lines

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

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

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

Quoted 3 lines

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

But cellphone user knows what he connected his charger to, and that's why it is useful to be able to lower the current. Even when you said "less is just stupid" I demonstrated it is not, at least in case when your power source is a battery.

From: Pavel Machek

Quoted 1 line

Sent: 18 April 2016 11:40

...

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 6 lines

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

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

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

Quoted 8 lines

Quoted 3 lines

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

But cellphone user knows what he connected his charger to, and that's why it is useful to be able to lower the current. Even when you said "less is just stupid" I demonstrated it is not, at least in case when your power source is a battery.

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

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

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

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

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

Hi Baolin,

Quoted 21 lines · Baolin Wang

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

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

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

Quoted 5 lines

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

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

--
balbi