Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1367072 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-03-30 12:20 +0200
    Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Mark Brown <broonie@kernel.org> - 2016-03-30 19:50 +0200
      Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-03-31 08:30 +0200
    Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-03-31 08:30 +0200
      Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-03-31 08:50 +0200
        Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-03-31 10:10 +0200
          Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-03-31 10:30 +0200
            Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-03-31 10:40 +0200
              Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-03-31 12:10 +0200
                Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-03-31 13:20 +0200
        Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Mark Brown <broonie@kernel.org> - 2016-03-31 19:10 +0200
          Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-04-01 07:50 +0200
            Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework Mark Brown <broonie@kernel.org> - 2016-04-01 16:20 +0200

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

[Multipart message — attachments visible in raw view] — view raw

Hi,

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

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

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

sure you don't depend on anything ?

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

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

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

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

> +#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)

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

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

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

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

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

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

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

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

likewise, why RW ? Missing doc.

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

why RW? Where's doc ?

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

why RW ? where's doc ?

> +static struct attribute *usb_charger_attrs[] = {

const ?

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

static ?

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

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

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

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

still not convinced this deserves to be a bus_type.

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

WARN() ?

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

WARN()

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

does this need EXPORT_SYMBOL_GPL() too ?

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

WARN()

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

WARN()

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

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

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

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

GPLv2 or GPLv2+ ??

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

usb_ is redundant. I'd prefer to:

#include <linux/usb/charger.h>

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

userland really doesn't need these two ?

-- 
balbi

[toc] | [next] | [standalone]

#1367495

[Multipart message — attachments visible in raw view] — view raw

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

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

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

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

> > +static DEVICE_ATTR_RW(sdp_limit);

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

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

[toc] | [prev] | [next] | [standalone]

#1367838

[Multipart message — attachments visible in raw view] — view raw

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

fair enough, seems like removal is the way.

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

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

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1367845

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

OK.

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

It just depends on USB_GADGET.

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

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

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

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

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

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

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

OK.

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

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

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

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

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

Same reason. I'll add the doc.

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

I'll add the doc in next version.

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

I'll add the doc in next version.

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

OK.

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

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

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

OK.

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

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

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

OK.


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

OK.

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

OK. I'll add it.

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

OK.

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

OK.

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

Make sense.

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

I'll modify the bus things.

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

OK.

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

Got it.

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

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

>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1367864

[Multipart message — attachments visible in raw view] — view raw

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

okay, that's cool :-)

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

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

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

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

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

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

and ARRAY_SIZE(arr) is not enough ?

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

I guess a class would fit better in this case.

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

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

a) you are connected to a dedicated charger

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Just trust the charging IC to do a good job.

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

indeed, thanks.

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

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1367941

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

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

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

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

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

OK.

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

OK.

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

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

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

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

>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1367975

[Multipart message — attachments visible in raw view] — view raw

Hi,

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

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

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

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1367986

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

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

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

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

>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1368132

[Multipart message — attachments visible in raw view] — view raw

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

int default_current_limit = 500;

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

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

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1368182

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

Make sense.

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

OK. Thanks.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1368456

[Multipart message — attachments visible in raw view] — view raw

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

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

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

IIRC Greg didn't want new classes?

[toc] | [prev] | [next] | [standalone]

#1369007

[Multipart message — attachments visible in raw view] — view raw

Hi,

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

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1369356

[Multipart message — attachments visible in raw view] — view raw

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

> > IIRC Greg didn't want new classes?

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web