From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753911AbbHGFsG (ORCPT ); Fri, 7 Aug 2015 01:48:06 -0400 Received: from mail-yk0-f175.google.com ([209.85.160.175]:36009 "EHLO mail-yk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752511AbbHGFsE (ORCPT ); Fri, 7 Aug 2015 01:48:04 -0400 MIME-Version: 1.0 In-Reply-To: <20150806163905.GA6907@kroah.com> References: <2ba023cbaa1dd96fd5e31e2f01cc0a599a3a5e55.1438844454.git.baolin.wang@linaro.org> <20150806163905.GA6907@kroah.com> Date: Fri, 7 Aug 2015 13:48:03 +0800 Message-ID: Subject: Re: [PATCH 1/2] gadget: Introduce the usb charger framework From: Baolin Wang To: Greg KH Cc: Felipe Balbi , Mark Brown , LKML , peter.chen@freescale.com, sojka@merica.cz, Alan Stern , andreas@gaisler.com, linux-usb@vger.kernel.org, device-mainlining@lists.linuxfoundation.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7 August 2015 at 00:39, Greg KH wrote: > On Thu, Aug 06, 2015 at 03:03:48PM +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/charger.c | 547 +++++++++++++++++++++++++++++++++++++++ >> include/linux/usb/usb_charger.h | 101 ++++++++ >> 2 files changed, 648 insertions(+) >> create mode 100644 drivers/usb/gadget/charger.c >> create mode 100644 include/linux/usb/usb_charger.h >> >> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c >> new file mode 100644 >> index 0000000..3ca0180 >> --- /dev/null >> +++ b/drivers/usb/gadget/charger.c >> @@ -0,0 +1,547 @@ >> +/* >> + * usb charger.c -- USB charger driver >> + * >> + * 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"? > I'll think about this and modify it. >> + */ >> + >> +#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 LIST_HEAD(usb_charger_list); >> +static DEFINE_MUTEX(usb_charger_list_lock); >> + >> +/* >> + * usb_charger_find_by_name - Get the usb charger device by name. >> + * @name - usb charger device name. >> + * >> + * notes: when this function walks the list and returns a charger >> + * it's dropped the lock which means that something else could come >> + * along and delete the charger before we dereference the pointer. >> + * It's very unlikely but it's a possibility so you should take care >> + * of it. >> + * Thus when you get the usb charger by name, you should call >> + * put_usb_charger() to derease the reference count of the usb charger. >> + * >> + * return the instance of usb charger device. >> + */ >> +struct usb_charger *usb_charger_find_by_name(char *name) >> +{ >> + struct usb_charger *uchger; >> + >> + if (!name) >> + return ERR_PTR(-EINVAL); >> + >> + mutex_lock(&usb_charger_list_lock); >> + list_for_each_entry(uchger, &usb_charger_list, entry) { >> + if (!strcmp(uchger->name, name)) { >> + get_usb_charger(uchger); >> + mutex_unlock(&usb_charger_list_lock); >> + return uchger; >> + } >> + } >> + mutex_unlock(&usb_charger_list_lock); >> + >> + return NULL; >> +} >> + >> +/* >> + * usb_charger_register_notify() - Register a notifiee to get notified by >> + * any attach status changes from the usb charger type detection. >> + * @uchger - the usb charger device which is monitored. >> + * @nb - a notifier block to be registered. >> + */ >> +void usb_charger_register_notify(struct usb_charger *uchger, >> + struct notifier_block *nb) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&uchger->lock, flags); >> + raw_notifier_chain_register(&uchger->uchger_nh, nb); >> + spin_unlock_irqrestore(&uchger->lock, flags); >> +} >> + >> +/* >> + * 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. >> + */ >> +void usb_charger_unregister_notify(struct usb_charger *uchger, >> + struct notifier_block *nb) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&uchger->lock, flags); >> + raw_notifier_chain_unregister(&uchger->uchger_nh, nb); >> + spin_unlock_irqrestore(&uchger->lock, flags); >> +} >> + >> +/* >> + * usb_charger_register_extcon_notifier() - Register a notifiee of the usb >> + * charger to get notified by any attach status changes from >> + * the extcon device. >> + * @uchger - the usb charger device. >> + * @edev - the extcon device. >> + * @extcon_id - extcon id. >> + */ >> +int usb_charger_register_extcon_notifier(struct usb_charger *uchger, >> + struct extcon_dev *edev, >> + unsigned int extcon_id) >> +{ >> + if (!uchger || !edev) >> + return -EINVAL; >> + >> + return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb); >> +} > > Why do we need wrappers around extcon? I thought extcon was supposed to > do all of this for us, why are we putting another layer on top of it? > OK, I'll remove the wrapper. >> +static void usb_charger_release(struct device *dev) >> +{ >> + struct usb_charger *uchger = dev_get_drvdata(dev); >> + >> + if (!atomic_dec_and_test(&uchger->count)) { >> + dev_err(dev, "The usb charger is still in use\n"); > > Why is the "count" different from the reference count? You shouldn't be > in this function if the reference count is not 0, so tie your "user" > count to this one. Having two different reference counts is a nightmare > and almost impossible to get right. And a huge red flag that the design > is incorrect. > Make sense of it. I'll fix that. >> + return; > > You can't "fail" a release call, so you just leaked memory all over the > floor here :( > Sorry, I'll think about here with the reference count. >> +/* >> + * usb_charger_register() - Register a new usb charger device. >> + * @uchger - the new usb charger device. > > No, you should create the new charger device, as this subsystem now owns > the life cycle. Don't rely on someone else to pass you an already > created structure. > Make sense. >> + * >> + */ >> +int usb_charger_register(struct device *dev, struct usb_charger *uchger) >> +{ >> + static atomic_t uchger_no = ATOMIC_INIT(-1); > > Use an idr/ida structure, don't try to roll your own logic here for > stuff that was long done for you. > OK. > >> + struct usb_charger *tmp; >> + 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; >> + dev_set_name(&uchger->dev, "usb-chger%lu", >> + (unsigned long)atomic_inc_return(&uchger_no)); >> + >> + ret = device_register(&uchger->dev); >> + if (ret) { >> + put_device(&uchger->dev); >> + return ret; >> + } >> + >> + dev_set_drvdata(&uchger->dev, uchger); >> + >> + mutex_lock(&usb_charger_list_lock); >> + list_for_each_entry(tmp, &usb_charger_list, entry) { >> + if (!(strcmp(tmp->name, uchger->name))) { >> + mutex_unlock(&usb_charger_list_lock); >> + ret = -EEXIST; >> + goto out; >> + } >> + } >> + list_add_tail(&uchger->entry, &usb_charger_list); > > Why do you need a separate list? This subsystem's bus structure should > own that list of devices, no need for a separate one (again, a huge red > flag that the design is not correct.) > > I stopped here. Please rebase on linux-next and resend. > OK, Very sorry for the email reply before. I'll modify the problems you point out. Very thanks for your comments. > thanks, > > greg k-h -- Baolin.wang Best Regards