Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@xxxxxxxxx> wrote:
> On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
>>
>> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
>>       struct usb_ep *(*match_ep)(struct usb_gadget *,
>>                       struct usb_endpoint_descriptor *,
>>                       struct usb_ss_ep_comp_descriptor *);
>> +     /* get the charger type */
>> +     enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
>>  };
>
> Since we already have get_charger_type callback at usb_charger
> structure, why we still need this API at usb_gadget_ops?

In case some users want to get charger type at gadget level.

>
>>
>>  /**
>> @@ -635,6 +638,8 @@ struct usb_gadget {
>>       unsigned                        out_epnum;
>>       unsigned                        in_epnum;
>>       struct usb_otg_caps             *otg_caps;
>> +     /* negotiate the power with the usb charger */
>> +     struct usb_charger              *charger;
>>
>>       unsigned                        sg_supported:1;
>>       unsigned                        is_otg:1;
>> @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget)
>>   * reporting how much power the device may consume.  For example, this
>>   * could affect how quickly batteries are recharged.
>>   *
>> + * It will also notify the USB charger how much power the device may
>> + * consume if there is a USB charger linking with the gadget.
>> + *
>>   * Returns zero on success, else negative errno.
>>   */
>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>  {
>> +     enum usb_charger_type type;
>> +
>> +     if (gadget->charger) {
>> +             type = usb_charger_get_type(gadget->charger);
>> +             usb_charger_set_cur_limit_by_type(gadget->charger, type, mA);
>> +     }
>> +
>
> You may do something redundant.
>
> - Charger detection only occurs at connection period, at other periods,
> we only change the current limit, and notify charger IC. That is to
> say, we may only need to save charger type and current limit at
> usb_charger structure, we don't need to distinguish all chargers
> type from time to time.

That's right. I just want to get the charger type as one parameter to
set current. The function is implemented as below:

enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
{
        enum usb_charger_type type;

        mutex_lock(&uchger->lock);
        type = uchger->type;
        mutex_unlock(&uchger->lock);

        return type;
}

>
> - The purpose of usb_gadget_vbus_draw design is notify charger IC too,
> so you can do set current limit and notify charger IC together at this
> API together, it has already covered all situations. We don't need to
> notify charger IC again when the gadget status has changed again.

It did not notify charger IC again. You are right,
usb_gadget_vbus_draw design will notify charger IC, so we want to
record the current in usb charger framework at the same time.

>
>>       if (!gadget->ops->vbus_draw)
>>               return -EOPNOTSUPP;
>>       return gadget->ops->vbus_draw(gadget, mA);
>> --
>> 1.7.9.5
>>
>> --
>> 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
>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards