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

[Baolin Wang]

On 31 March 2016 at 14:42, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>>>> +#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 ?

Make sense. I'll remove the 'DEFAULT_CUR_PROTECT' macro.

>
>>>> +#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.

Make sense. But these are just default values. They can be changed
safely by power driver with 'usb_charger_set_cur_limit_by_type()'
function to set 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 ?

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.

OK.

>
>>>> +{
>>>> +     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.

That sounds reasonable. Mark what do you think? Thanks.

>
> Just trust the charging IC to do a good job.
>
>>>> +
>>>> +/* 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.

Make sense. I can introduce some sysfs files for userspace. Thanks for
your comments.

>
> --
> balbi



-- 
Baolin.wang
Best Regards