Hi,
On Sun, Jul 03, 2011 at 05:29:32PM +0300, Amit Blay wrote:
> 1. Add missing definitions in ch9.h for requests tarageted to an
typo... s/tarageted/targeted
> 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.
> 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 ;-)
> Signed-off-by: Amit Blay <ablay@xxxxxxxxxxxxxx>
>
> ---
> drivers/usb/gadget/udc-core.c | 2 +-
> drivers/usb/gadget/zero.c | 6 +++++-
> include/linux/usb/ch9.h | 8 ++++++++
> include/linux/usb/gadget.h | 35 +++++++++++++++++++++++++++--------
> 4 files changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 05ba472..beb7e89 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -347,7 +347,7 @@ static ssize_t usb_udc_srp_store(struct device *dev,
> struct usb_udc *udc = dev_get_drvdata(dev);
>
> if (sysfs_streq(buf, "1"))
> - usb_gadget_wakeup(udc->gadget);
> + usb_gadget_wakeup(udc->gadget, 0);
>
> return n;
> }
> diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
> index 00e2fd2..a5d13c6 100644
> --- a/drivers/usb/gadget/zero.c
> +++ 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).
> INFO(cdev, "%s --> %d\n", __func__, status);
> }
> }
> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
> index 0fd3fbd..404c10e 100644
> --- a/include/linux/usb/ch9.h
> +++ b/include/linux/usb/ch9.h
> @@ -142,7 +142,13 @@
> #define USB_DEVICE_LTM_ENABLE 50 /* dev may send LTM */
> #define USB_INTRF_FUNC_SUSPEND 0 /* function suspend */
>
> +/*
> + * Function Suspend Options as defined by USB 3.0
> + * See USB 3.0 spec Table 9-7
> + */
> #define USB_INTR_FUNC_SUSPEND_OPT_MASK 0xFF00
> +#define USB_INTR_FUNC_SUSPEND_SUSP 1 /* function suspend option */
> +#define USB_INTR_FUNC_SUSPEND_RWAKE_EN 2 /* function wake enabled option */
>
> #define USB_ENDPOINT_HALT 0 /* IN/OUT will STALL */
>
> @@ -150,6 +156,8 @@
> #define USB_DEV_STAT_U1_ENABLED 2 /* transition into U1 state */
> #define USB_DEV_STAT_U2_ENABLED 3 /* transition into U2 state */
> #define USB_DEV_STAT_LTM_ENABLED 4 /* Latency tolerance messages */
> +#define USB_INTR_STAT_RWAKE_CAP 0 /* Function wake capable */
> +#define USB_INTR_STAT_RWAKE_EN 1 /* Function wake enabled */
>
> /**
> * struct usb_ctrlrequest - SETUP data for a USB device control request
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 087f4b9..3ebbc11 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -458,7 +458,11 @@ struct usb_gadget_ops {
> int (*pullup) (struct usb_gadget *, int is_on);
> int (*ioctl)(struct usb_gadget *,
> unsigned code, unsigned long param);
> +
> + /* USB 3.0 additions */
this comment is not part of this patch ;-) (nitpicking, I know)
> void (*get_config_params)(struct usb_dcd_config_params *);
> + int (*func_wakeup)(struct usb_gadget *, int interface_id);
> +
> int (*udc_start)(struct usb_gadget *,
> struct usb_gadget_driver *);
> int (*udc_stop)(struct usb_gadget *,
> @@ -607,21 +611,36 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
> /**
> * usb_gadget_wakeup - tries to wake up the host connected to this gadget
> * @gadget: controller used to wake up the host
> + * @interface_id: id of the first interface of the function that
> + * triggered the wake up
> *
> * Returns zero on success, else negative error code if the hardware
> * doesn't support such attempts, or its support has not been enabled
> * by the usb host. Drivers must return device descriptors that report
> * their ability to support this, or hosts won't enable it.
> + * For Super-Speed devices only, the gadget should provide the
> + * id of the first interface that triggered the wake up
> + * (function wake). For non Super-Speed devices, the value of
> + * this parameter doesn't matter.
> *
> - * This may also try to use SRP to wake the host and start enumeration,
> - * even if OTG isn't otherwise in use. OTG devices may also start
> - * remote wakeup even when hosts don't explicitly enable it.
> + * This may also try to use SRP to wake the host and start
> + * enumeration, even if OTG isn't otherwise in use. OTG devices
> + * may also start remote wakeup even when hosts don't explicitly
> + * enable it.
> */
> -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.
--
balbi
Attachment:
signature.asc
Description: Digital signature
