Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755421AbbHNP10 (ORCPT ); Fri, 14 Aug 2015 11:27:26 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:48290 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755140AbbHNP1Y (ORCPT ); Fri, 14 Aug 2015 11:27:24 -0400 Date: Fri, 14 Aug 2015 08:27:23 -0700 From: Greg KH To: Baolin Wang Cc: balbi@ti.com, broonie@kernel.org, linus.walleij@linaro.org, linux-kernel@vger.kernel.org, peter.chen@freescale.com, sojka@merica.cz, stern@rowland.harvard.edu, r.baldyga@samsung.com, yoshihiro.shimoda.uh@renesas.com, linux-usb@vger.kernel.org, device-mainlining@lists.linuxfoundation.org, sre@kernel.org, dbaryshkov@gmail.com, dwmw2@infradead.org, sameo@linux.intel.com, lee.jones@linaro.org, patches@opensource.wolfsonmicro.com, linux-pm@vger.kernel.org Subject: Re: [PATCH v2 2/3] gadget: Introduce the usb charger framework Message-ID: <20150814152723.GA17892@kroah.com> References: <0c41e21239f69c4e1aa89aba020c91edfc17c4e8.1439519413.git.baolin.wang@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0c41e21239f69c4e1aa89aba020c91edfc17c4e8.1439519413.git.baolin.wang@linaro.org> User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 20971 Lines: 735 On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote: > This patch introduces the usb charger driver based on usb gadget that > makes an enhancement to a power driver. It works well in practice but > that requires a system with suitable hardware. > > The basic conception of the usb charger is that, when one usb charger > is added or removed by reporting from the usb gadget state change or > the extcon device state change, the usb charger will report to power > user to set the current limitation. > > The usb charger will register notifiees on the usb gadget or the extcon > device to get notified the usb charger state. > > Power user will register a notifiee on the usb charger to get notified > by status changes from the usb charger. It will report to power user > to set the current limitation when detecting the usb charger is added > or removed from extcon device state or usb gadget state. > > Signed-off-by: Baolin Wang > --- > drivers/usb/gadget/Kconfig | 7 + > drivers/usb/gadget/Makefile | 1 + > drivers/usb/gadget/charger.c | 561 +++++++++++++++++++++++++++++++++++++++ > include/linux/usb/usb_charger.h | 145 ++++++++++ > 4 files changed, 714 insertions(+) > create mode 100644 drivers/usb/gadget/charger.c > create mode 100644 include/linux/usb/usb_charger.h > > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig > index bcf83c0..3d2b959 100644 > --- a/drivers/usb/gadget/Kconfig > +++ b/drivers/usb/gadget/Kconfig > @@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS > a module parameter as well. > If unsure, say 2. > > +config USB_CHARGER > + bool "USB charger support" > + help > + The usb charger driver based on the usb gadget that makes an > + enhancement to a power driver which can set the current limitation > + when the usb charger is added or removed. > + > source "drivers/usb/gadget/udc/Kconfig" > > # > diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile > index 598a67d..1e421c1 100644 > --- a/drivers/usb/gadget/Makefile > +++ b/drivers/usb/gadget/Makefile > @@ -10,3 +10,4 @@ libcomposite-y := usbstring.o config.o epautoconf.o > libcomposite-y += composite.o functions.o configfs.o u_f.o > > obj-$(CONFIG_USB_GADGET) += udc/ function/ legacy/ > +obj-$(CONFIG_USB_CHARGER) += charger.o > diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c > new file mode 100644 > index 0000000..f24f7b7 > --- /dev/null > +++ b/drivers/usb/gadget/charger.c > @@ -0,0 +1,561 @@ > +/* > + * usb charger.c -- USB charger driver I think your company also wants a copyright line here. Not that it means anything at all, but some lawyers love cargo-cult text blobs. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. I have to ask, do you really mean "any later version"? > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DEFAULT_CUR_PROTECT (50) > +#define DEFAULT_SDP_CUR_LIMIT (500 - DEFAULT_CUR_PROTECT) > +#define DEFAULT_DCP_CUR_LIMIT (1500 - DEFAULT_CUR_PROTECT) > +#define DEFAULT_CDP_CUR_LIMIT (1500 - DEFAULT_CUR_PROTECT) > +#define DEFAULT_ACA_CUR_LIMIT (1500 - DEFAULT_CUR_PROTECT) > + > +static DEFINE_IDA(usb_charger_ida); > +static struct bus_type usb_charger_subsys = { > + .name = "usb-charger", > + .dev_name = "usb-charger", > +}; > + > +static struct usb_charger *dev_to_uchger(struct device *udev) > +{ > + return container_of(udev, struct usb_charger, dev); > +} > + > +static ssize_t usb_charger_cur_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct usb_charger *uchger = dev_to_uchger(dev); > + > + return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n", > + uchger->cur_limit.sdp_cur_limit, > + uchger->cur_limit.dcp_cur_limit, > + uchger->cur_limit.cdp_cur_limit, > + uchger->cur_limit.aca_cur_limit); > +} > + > +static ssize_t usb_charger_cur_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct usb_charger *uchger = dev_to_uchger(dev); > + struct usb_charger_cur_limit cur; > + int ret; > + > + ret = sscanf(buf, "%d %d %d %d", > + &cur.sdp_cur_limit, &cur.dcp_cur_limit, > + &cur.cdp_cur_limit, &cur.aca_cur_limit); > + if (ret == 0) > + return -EINVAL; > + > + ret = usb_charger_set_cur_limit(uchger, &cur); > + if (ret < 0) > + return ret; > + > + return count; > +} > +static DEVICE_ATTR(cur_limit, 0644, usb_charger_cur_show, usb_charger_cur_store); DEVICE_ATTR_RW()? > + > +static struct attribute *uchger_attributes[] = { > + &dev_attr_cur_limit.attr, > + NULL > +}; > + > +static const struct attribute_group uchger_attr_group = { > + .attrs = uchger_attributes, > +}; ATTRIBUTE_GROUPS()? > + > +/* > + * usb_charger_find_by_name - Get the usb charger device by name. > + * @name - usb charger device name. > + * > + * return the instance of usb charger device, the device must be > + * released with usb_charger_put(). > + */ > +struct usb_charger *usb_charger_find_by_name(const char *name) > +{ > + struct device *udev; > + > + if (!name) > + return ERR_PTR(-EINVAL); > + > + udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name); > + if (!udev) > + return ERR_PTR(-ENODEV); > + > + return dev_to_uchger(udev); > +} > + > +/* > + * usb_charger_get() - Reference a usb charger. > + * @uchger - usb charger > + */ > +struct usb_charger *usb_charger_get(struct usb_charger *uchger) > +{ > + return (uchger && get_device(&uchger->dev)) ? uchger : NULL; > +} > + > +/* > + * usb_charger_put() - Dereference a usb charger. > + * @uchger - charger to release > + */ > +void usb_charger_put(struct usb_charger *uchger) > +{ > + if (uchger) > + put_device(&uchger->dev); > +} > + > +/* > + * usb_charger_register_notify() - Register a notifiee to get notified by > + * any attach status changes from the usb charger detection. > + * @uchger - the usb charger device which is monitored. > + * @nb - a notifier block to be registered. > + */ > +int usb_charger_register_notify(struct usb_charger *uchger, > + struct notifier_block *nb) > +{ > + int ret; > + > + if (!uchger || !nb) > + return -EINVAL; > + > + mutex_lock(&uchger->lock); > + ret = raw_notifier_chain_register(&uchger->uchger_nh, nb); > + > + /* Generate an initial notify so users start in the right state */ > + if (!ret) { > + usb_charger_detect_type(uchger); > + raw_notifier_call_chain(&uchger->uchger_nh, > + usb_charger_get_cur_limit(uchger), > + uchger); > + } > + mutex_unlock(&uchger->lock); > + > + return ret; > +} > + > +/* > + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger. > + * @uchger - the usb charger device which is monitored. > + * @nb - a notifier block to be unregistered. > + */ > +int usb_charger_unregister_notify(struct usb_charger *uchger, > + struct notifier_block *nb) > +{ > + int ret; > + > + if (!uchger || !nb) > + return -EINVAL; > + > + mutex_lock(&uchger->lock); > + ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb); > + mutex_unlock(&uchger->lock); > + > + return ret; > +} > + > +/* > + * usb_charger_detect_type() - Get the usb charger type by the callback > + * which is implemented by gadget operations. > + * @uchger - the usb charger device. > + * > + * return the usb charger type. > + */ > +enum usb_charger_type > +usb_charger_detect_type(struct usb_charger *uchger) > +{ > + if (uchger->gadget && uchger->gadget->ops > + && uchger->gadget->ops->get_charger_type) > + uchger->type = > + uchger->gadget->ops->get_charger_type(uchger->gadget); > + else > + uchger->type = UNKNOWN_TYPE; > + > + return uchger->type; > +} > + > +/* > + * usb_charger_set_cur_limit() - Set the current limitation. > + * @uchger - the usb charger device. > + * @cur_limit_set - the current limitation. > + */ > +int usb_charger_set_cur_limit(struct usb_charger *uchger, > + struct usb_charger_cur_limit *cur_limit_set) > +{ > + if (!uchger || !cur_limit_set) > + return -EINVAL; > + > + uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit; > + uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit; > + uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit; > + uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit; > + return 0; > +} > + > +/* > + * usb_charger_get_cur_limit() - Get the current limitation by > + * different usb charger type. > + * @uchger - the usb charger device. > + * > + * return the current limitation to set. > + */ > +unsigned int > +usb_charger_get_cur_limit(struct usb_charger *uchger) > +{ > + enum usb_charger_type uchger_type = uchger->type; > + unsigned int cur_limit; > + > + switch (uchger_type) { > + case SDP_TYPE: > + cur_limit = uchger->cur_limit.sdp_cur_limit; > + break; > + case DCP_TYPE: > + cur_limit = uchger->cur_limit.dcp_cur_limit; > + break; > + case CDP_TYPE: > + cur_limit = uchger->cur_limit.cdp_cur_limit; > + break; > + case ACA_TYPE: > + cur_limit = uchger->cur_limit.aca_cur_limit; > + break; > + default: > + return 0; > + } > + > + return cur_limit; > +} > + > +/* > + * usb_charger_notifier_others() - It will notify other device registered > + * on usb charger when the usb charger state is changed. > + * @uchger - the usb charger device. > + * @state - the state of the usb charger. > + */ > +static void > +usb_charger_notify_others(struct usb_charger *uchger, > + enum usb_charger_state state) > +{ > + mutex_lock(&uchger->lock); > + uchger->state = state; > + > + switch (state) { > + case USB_CHARGER_PRESENT: > + usb_charger_detect_type(uchger); > + raw_notifier_call_chain(&uchger->uchger_nh, > + usb_charger_get_cur_limit(uchger), > + uchger); > + break; > + case USB_CHARGER_REMOVE: > + uchger->type = UNKNOWN_TYPE; > + raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger); > + break; > + default: > + dev_warn(&uchger->dev, "Unknown USB charger state: %d\n", > + state); > + break; > + } > + mutex_unlock(&uchger->lock); > +} > + > +/* > + * usb_charger_plug_by_extcon() - The notifier call function which is registered > + * on the extcon device. > + * @nb - the notifier block that notified by extcon device. > + * @state - the extcon device state. > + * @data - here specify a extcon device. > + * > + * return the notify flag. > + */ > +static int > +usb_charger_plug_by_extcon(struct notifier_block *nb, > + unsigned long state, void *data) > +{ > + struct usb_charger_nb *extcon_nb = > + container_of(nb, struct usb_charger_nb, nb); > + struct usb_charger *uchger = extcon_nb->uchger; > + enum usb_charger_state uchger_state; > + > + if (!uchger) > + return NOTIFY_BAD; > + > + /* Report event to power to setting the current limitation > + * for this usb charger when one usb charger is added or removed > + * with detecting by extcon device. > + */ > + if (state) > + uchger_state = USB_CHARGER_PRESENT; > + else > + uchger_state = USB_CHARGER_REMOVE; > + > + usb_charger_notify_others(uchger, uchger_state); > + > + return NOTIFY_OK; > +} > + > +/* > + * usb_charger_plug_by_gadget() - Set the usb charger current limitation > + * according to the usb gadget device state. > + * @nb - the notifier block that notified by usb gadget device. > + * @state - the usb gadget state. > + * @data - here specify a usb gadget device. > + */ > +static int > +usb_charger_plug_by_gadget(struct notifier_block *nb, > + unsigned long state, void *data) > +{ > + struct usb_gadget *gadget = (struct usb_gadget *)data; > + struct usb_charger *uchger = gadget->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 state is changed > + * with detecting by usb gadget state. > + */ > + if (uchger->old_gadget_state != state) { > + uchger->old_gadget_state = state; > + > + if (state >= USB_STATE_ATTACHED) > + uchger_state = USB_CHARGER_PRESENT; > + else if (state == USB_STATE_NOTATTACHED) > + uchger_state = USB_CHARGER_REMOVE; > + else > + uchger_state = USB_CHARGER_DEFAULT; > + > + usb_charger_notify_others(uchger, uchger_state); > + } > + > + return NOTIFY_OK; > +} > + > +static int devm_uchger_dev_match(struct device *dev, void *res, void *data) > +{ > + struct usb_charger **r = res; > + > + if (WARN_ON(!r || !*r)) > + return 0; > + > + return *r == data; > +} > + > +static void usb_charger_release(struct device *dev) > +{ > + struct usb_charger *uchger = dev_get_drvdata(dev); > + > + kfree(uchger->name); > + kfree(uchger); > +} > + > +/* > + * usb_charger_unregister() - Unregister a usb charger device. > + * @uchger - the usb charger to be unregistered. > + */ > +static int usb_charger_unregister(struct usb_charger *uchger) > +{ > + if (!uchger) > + return -EINVAL; > + > + sysfs_remove_group(&uchger->dev.kobj, &uchger_attr_group); > + device_unregister(&uchger->dev); > + > + return 0; > +} > + > +static void devm_uchger_dev_unreg(struct device *dev, void *res) > +{ > + usb_charger_unregister(*(struct usb_charger **)res); > +} > + > +void devm_usb_charger_unregister(struct device *dev, > + struct usb_charger *uchger) > +{ > + devres_release(dev, devm_uchger_dev_unreg, > + devm_uchger_dev_match, uchger); > +} > + > +/* > + * usb_charger_register() - Register a new usb charger device > + * which is created by the usb charger framework. > + * @uchger - the new usb charger device. > + */ > +static int usb_charger_register(struct device *dev, struct usb_charger *uchger) > +{ > + int ret; > + > + if (!uchger) { > + dev_err(dev, "no device provided for charger\n"); > + return -EINVAL; > + } > + > + uchger->dev.parent = dev; > + uchger->dev.release = usb_charger_release; > + uchger->dev.bus = &usb_charger_subsys; > + > + ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL); > + if (ret < 0) { > + dev_err(dev, "get usb charger id failed\n"); > + return ret; > + } > + > + 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; > + > + ret = sysfs_create_group(&uchger->dev.kobj, &uchger_attr_group); And you just raced with userspace, and lost :( Assign the group to the device _before_ you tell userspace about it. The big hint here that is wrong is that you have to call a sysfs_* function for a struct device. There is no device_create_group() call for a reason. > + if (ret) > + goto fail_create_files; > + > + return 0; > + > +fail_create_files: > + device_unregister(&uchger->dev); > +fail_register: > + put_device(&uchger->dev); > + ida_simple_remove(&usb_charger_ida, uchger->id); > + uchger->id = -1; > + dev_err(dev, "Failed to register usb charger (%s)\n", > + uchger->name); > + 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; > + char buf[100]; That's a lot of stack, why? > + char *str; > + int ret; > + > + if (!ugadget) > + return -EINVAL; > + > + uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger), > + GFP_KERNEL); No, you are creating a new struct device, you can't assign its resources to another struct device, that breaks all of the lifetime rules. kzalloc() only please. > + if (!uchger) > + return -ENOMEM; > + > + sprintf(buf, "usb-charger.%s", ugadget->name); > + str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1), > + GFP_KERNEL); Why have a string when you already have a name for the device, in the device itself? What is this all for? > + if (!str) > + return -ENOMEM; > + > + strcpy(str, buf); > + uchger->name = str; > + uchger->type = UNKNOWN_TYPE; > + uchger->state = USB_CHARGER_DEFAULT; > + uchger->id = -1; > + uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT; > + uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT; > + uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT; > + uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT; > + > + mutex_init(&uchger->lock); > + INIT_LIST_HEAD(&uchger->entry); > + RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh); > + > + /* register a notifier on a extcon device if it is exsited */ > + edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0); > + if (!IS_ERR_OR_NULL(edev)) { > + uchger->extcon_dev = edev; > + uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon; > + uchger->extcon_nb.uchger = uchger; > + extcon_register_notifier(edev, EXTCON_USB, &uchger->extcon_nb.nb); > + } > + > + /* register a notifier on a usb gadget device */ > + uchger->gadget = ugadget; > + ugadget->uchger = uchger; > + uchger->old_gadget_state = ugadget->state; > + uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget; > + usb_gadget_register_notify(ugadget, &uchger->gadget_nb); > + > + /* register a new usb charger */ > + ret = usb_charger_register(&ugadget->dev, uchger); > + if (ret) > + goto fail; > + > + return 0; > + > +fail: > + if (uchger->extcon_dev) > + extcon_unregister_notifier(uchger->extcon_dev, > + EXTCON_USB, &uchger->extcon_nb.nb); > + > + usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb); > + return ret; > +} > + > +int usb_charger_exit(struct usb_gadget *ugadget) > +{ > + struct usb_charger *uchger = ugadget->uchger; > + > + if (!uchger) > + return -EINVAL; > + > + if (uchger->extcon_dev) > + extcon_unregister_notifier(uchger->extcon_dev, > + EXTCON_USB, &uchger->extcon_nb.nb); > + > + usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb); > + ida_simple_remove(&usb_charger_ida, uchger->id); > + > + return usb_charger_unregister(uchger); > +} > + > +static int __init usb_charger_sysfs_init(void) > +{ > + return subsys_system_register(&usb_charger_subsys, NULL); > +} > +core_initcall(usb_charger_sysfs_init); > + > +MODULE_AUTHOR("Baolin Wang "); > +MODULE_DESCRIPTION("USB charger driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h > new file mode 100644 > index 0000000..8cbfaae > --- /dev/null > +++ b/include/linux/usb/usb_charger.h > @@ -0,0 +1,145 @@ > +#ifndef __LINUX_USB_CHARGER_H__ > +#define __LINUX_USB_CHARGER_H__ > + > +#include > +#include > +#include > +#include > + > +/* USB charger type: > + * SDP (Standard Downstream Port) > + * DCP (Dedicated Charging Port) > + * CDP (Charging Downstream Port) > + * ACA (Accessory Charger Adapters) > + */ > +enum usb_charger_type { > + UNKNOWN_TYPE, > + SDP_TYPE, > + DCP_TYPE, > + CDP_TYPE, > + ACA_TYPE, > +}; > + > +/* USB charger state */ > +enum usb_charger_state { > + USB_CHARGER_DEFAULT, > + USB_CHARGER_PRESENT, > + USB_CHARGER_REMOVE, > +}; > + > +/* Current limitation by charger type */ > +struct usb_charger_cur_limit { > + unsigned int sdp_cur_limit; > + unsigned int dcp_cur_limit; > + unsigned int cdp_cur_limit; > + unsigned int aca_cur_limit; > +}; > + > +struct usb_charger_nb { > + struct notifier_block nb; > + struct usb_charger *uchger; > +}; > + > +struct usb_charger { > + /* Internal data. Please do not set. */ Heh, as if people read comments :( > + const char *name; This isn't needed, use the one in your struct device. > + struct device dev; > + struct raw_notifier_head uchger_nh; > + struct list_head entry; Why do you need another list? The device already is on a list. > + struct mutex lock; What is this protecting, document it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/