Hi Felipe,
>> Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).
>
> if it's targeted to an interface, why don't you just let the gadget
> driver handle it ? composite.c tracks all functions already and we
> should just call function->setup() to let the correct function handle
> this.
Your question is regarding why we added the function->func_suspend &
function->get_status callbacks in the first place? (I'm asking because
this is not covered by this patch...)
If we let both requests to be handled by function->setup(), then to
support SuperSpeed, we'll have to change each and every function's setup()
to respond back with a correct default value.
The advantage of adding function->func_suspend & function->get_status is
that if the function doesn't supply those functions, the default can be
handled by the composite setup() function.
>> 2. Add func_wakeup api to usb_gadget_ops.
>> Super-Speed device must provide interface number to the UDC when
>> it triggers remote wakeup (function wake).
>> The func_wakeup api is used instead of the wakeup api, when the
>> gadget is connected in Super-Speed. The wakeup api is left as is,
>> and it is used when the gadget is connected in High-Speed. Therefore,
>> no change is required in non Super-Speed UDCs.
> first of all, this needs to be splitted. You shouldn't do more than one
> thing in a patch ;-)
OK, I will split the patch.
>> +++ b/drivers/usb/gadget/zero.c
>> @@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
>> * because of some direct user request.
>> */
>> if (g->speed != USB_SPEED_UNKNOWN) {
>> - int status = usb_gadget_wakeup(g);
>> + /*
>> + * The single interface of the current configuration
>> + * triggers the wakeup.
>> + */
>> + int status = usb_gadget_wakeup(g, 1);
>
> no no, I think this should be handled by the function itself (f_*.c).
Yes you are right, the function should handle this. The next patch
(usb:gadget: SuperSpeed function power management testing support) adds
this exact capability to f_sourcesink. The function invokes the UDC's
func_wake callback.
But in this change above, I tried (with minimal changes) to keep the
current functionality of gadget zero's autoresume, which uses
usb_gadget_wakeup(). Since there is no device remote wakeup in SuperSpeed,
only function wake, calling usb_gadget_wakeup() has no real meaning in
non-SuperSpeed speeds.
The complete solution will be to move the autoresume feature from gadget
zero into the functions, f_sourcesink & f_loopback. This is the way wakeup
should be done in SuperSpeed, as I understand it. That means that both
functions will arm a timer on device suspend. When the timer expires, in
SuperSpeed it should call the UDC's func_wake callback. For lower speeds,
it should call usb_gadget_wakeup() to trigger device remote wakeup.
What do you think?
>> -static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
>> +static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int
>> interface_id)
>> {
>> - if (!gadget->ops->wakeup)
>> - return -EOPNOTSUPP;
>> - return gadget->ops->wakeup(gadget);
>> + if (gadget->speed == USB_SPEED_SUPER) {
>> + if (!gadget->ops->func_wakeup)
>> + return -EOPNOTSUPP;
>> +
>> + return gadget->ops->func_wakeup(gadget, interface_id);
>
> I really don't like this... You're just abusing this interface. Either
> add a separate one (which I still don't think it's the right approach)
> or just let the gadget driver handle it, meaning that composite.c would
> call into f_*.c and the drivers which don't use composite.c will handle
> it their own way.
This change is meant to keep usb_gadget_wakeup() API backwards compatible,
meaning that this API will still work in SuperSpeed.
Of course, if a new USB class will utilize function wake, the new function
will call gadget->ops->func_wakeup().
The solution I suggested above will address this comment as well, but the
downside is that it is not backward compatible, meaning, it requires to
change each gadget that is using usb_gadget_wakeup().
Thanks,
Amit.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
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
