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

[Felipe Balbi]

Baolin Wang <baolin.wang@xxxxxxxxxx> 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

Attachment: signature.asc
Description: PGP signature