Re: [RFC/PATCH 3/3] usb:gadget: SuperSpeed function power management testing support

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6422 bytes --]

On Sun, Jul 03, 2011 at 05:29:33PM +0300, Amit Blay wrote:
> 1. Fix dummy_hcd to let the function handle GET_STATUS(Interface) request.
> 2. Fix a bug in f_loopback Gadget which updated the bmAttribute of
> f_sourcesink instead of f_loopback.
> 3. Update f_sourcesink Gadget to support function suspend & wakeup.
> This is done by adding get_status() & func_suspend() handlers.
> The current status of the function is controlable via debug FS:
> (wakeup capable, wakeup enabled, suspended).
> 
> Signed-off-by: Amit Blay <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> ---
>  drivers/usb/gadget/dummy_hcd.c    |    3 +-
>  drivers/usb/gadget/f_loopback.c   |    2 +-
>  drivers/usb/gadget/f_sourcesink.c |  162 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 165 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index e755a9d..7f6a2d8 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -1496,7 +1496,8 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
>  					   Dev_InRequest) {
>  					buf[0] = (u8)dum->devstatus;
>  				} else
> -					buf[0] = 0;
> +					/* Let the function handle this */
> +					break;
>  			}
>  			if (urb->transfer_buffer_length > 1)
>  				buf[1] = 0;
> diff --git a/drivers/usb/gadget/f_loopback.c b/drivers/usb/gadget/f_loopback.c
> index ca660d4..75bac6d 100644
> --- a/drivers/usb/gadget/f_loopback.c
> +++ b/drivers/usb/gadget/f_loopback.c
> @@ -427,7 +427,7 @@ int __init loopback_add(struct usb_composite_dev *cdev, bool autoresume)
>  
>  	/* support autoresume for remote wakeup testing */
>  	if (autoresume)
> -		sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +		loopback_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;

this change doesn't belong here.

> diff --git a/drivers/usb/gadget/f_sourcesink.c b/drivers/usb/gadget/f_sourcesink.c
> index 5247f07..5c5da19 100644
> --- a/drivers/usb/gadget/f_sourcesink.c
> +++ b/drivers/usb/gadget/f_sourcesink.c
> @@ -59,6 +59,12 @@ struct f_sourcesink {
>  
>  	struct usb_ep		*in_ep;
>  	struct usb_ep		*out_ep;
> +
> +	/* Function Power Management */
> +	bool			    func_suspended;
> +	bool			    func_wakeup_capable;
> +	bool			    func_wakeup_enabled;
> +	struct			    device dev;

this should *NOT* have a device of its own. And the way you tabified is
wrong.

>  };
>  
>  static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
> @@ -191,6 +197,79 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
>  	NULL,
>  };
>  
> +/*************************** DEVICE ATTRIBUTES ***************************/
> +
> +static ssize_t f_sourcesink_show_func_suspend(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_suspended);
> +}
> +
> +static ssize_t f_sourcesink_show_func_wakeup_enabled(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_wakeup_enabled);
> +}
> +
> +static ssize_t f_sourcesink_show_func_wakeup_capable(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_wakeup_capable);
> +}
> +
> +static ssize_t f_sourcesink_store_func_wakeup_capable(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
> +	unsigned long func_wakeup_capable;
> +
> +	/* Allows changing function wakeup capable field from the file system */
> +	if (strict_strtoul(buf, 2, &func_wakeup_capable))
> +		return -EINVAL;
> +	ss->func_wakeup_capable = (bool)func_wakeup_capable;
> +	return count;
> +}
> +
> +static ssize_t f_sourcesink_store_func_wakeup_trigger(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
> +
> +	/* Allows trigerring function wakeup from the file system */
> +	if (!ss->func_wakeup_capable || !ss->func_wakeup_enabled)
> +		return -EINVAL;
> +
> +	if (usb_gadget_wakeup(ss->function.config->cdev->gadget,
> +				       source_sink_intf.bInterfaceNumber) < 0)
> +		return -EINVAL;
> +	return count;
> +}

why didn't you do this for composite.c ? Then all function drivers will
be able to be tested.

> @@ -199,6 +278,7 @@ sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
>  	struct usb_composite_dev *cdev = c->cdev;
>  	struct f_sourcesink	*ss = func_to_ss(f);
>  	int	id;
> +	int	result = 0;
>  
>  	/* allocate interface ID(s) */
>  	id = usb_interface_id(c, f);
> @@ -243,12 +323,66 @@ autoconf_fail:
>  	    (gadget_is_superspeed(c->cdev->gadget) ? "super" :
>  	     (gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
>  			f->name, ss->in_ep->name, ss->out_ep->name);
> +
> +	ss->dev.parent = &cdev->gadget->dev;
> +	ss->dev.release = sourcesink_release;
> +	dev_set_name(&ss->dev, "sourcesink");
> +
> +	result = device_register(&ss->dev);
> +	if (result) {
> +		ERROR(cdev, "failed to register: %d\n", result);
> +		goto err_device_register;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_suspend);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_suspend_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_enabled);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_enabled_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_capable);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_capable_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_trigger);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_trigger_file;
> +	}

dude ?!? Make a sysfs group ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]