On 7 August 2015 at 00:39, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
