Date:	Fri, 7 Aug 2015 13:48:03 +0800
From:	Baolin Wang <baolin.wang@...aro.org>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	Felipe Balbi <balbi@...com>, Mark Brown <broonie@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>, peter.chen@...escale.com,
	sojka@...ica.cz, Alan Stern <stern@...land.harvard.edu>,
	andreas@...sler.com, linux-usb@...r.kernel.org,
	device-mainlining@...ts.linuxfoundation.org
Subject: Re: [PATCH 1/2] gadget: Introduce the usb charger framework

On 7 August 2015 at 00:39, Greg KH <gregkh@...uxfoundation.org> 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 <baolin.wang@...aro.org>
>> ---
>>  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 <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/extcon.h>
>> +#include <linux/export.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/usb_charger.h>
>> +
>> +#define DEFAULT_CUR_PROTECT  (50)
>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +
>> +static 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists