Re: [RFC/PATCH v2 2/3] usb: dummy_hcd code simplification

[Alan Stern]

On Mon, 18 Oct 2010, Tatyana Brokhman wrote:

> Take handling of the control requests out from dummy_timer to a different
> function.

This is basically okay but it has a few problems.

> 
> Signed-off-by: Tatyana Brokhman <tlinder@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/gadget/dummy_hcd.c |  284 ++++++++++++++++++++++++----------------
>  1 files changed, 168 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index dc65462..7640e06 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -1195,6 +1195,173 @@ static struct dummy_ep *find_endpoint (struct dummy *dum, u8 address)
>  #define Ep_Request	(USB_TYPE_STANDARD | USB_RECIP_ENDPOINT)
>  #define Ep_InRequest	(Ep_Request | USB_DIR_IN)
>  
> +
> +/**
> + * This function handles all control transferes
> + *
> + * @param dum - pointer to dummy (the_controller)
> + * @param urb - the urb request to handle
> + * @param status - pointer to request handling status
> + * @param master - pointer to the dummy master this request was
> + *		 received for
> + */

The kerneldoc format is still wrong.  Please fix it.  What is all this
"@param" stuff?  And what is "master"?

> +static int handle_control_request(struct dummy *dum, struct urb *urb,
> +				  int *status)
> +{
> +	struct usb_ctrlrequest setup;
> +	struct dummy_ep		*ep2;
> +	int			ret_val = 1;
> +	unsigned	w_index;
> +	unsigned	w_value;
> +
> +	setup = *(struct usb_ctrlrequest *) urb->setup_packet;

You should pass the "setup" variable as a pointer instead of making it
a local variable.

> +	w_index = le16_to_cpu(setup.wIndex);
> +	w_value = le16_to_cpu(setup.wValue);
> +	switch (setup.bRequest) {
> +	case USB_REQ_SET_ADDRESS:
> +		if (setup.bRequestType != Dev_Request)
> +			break;
> +		dum->address = w_value;
> +		*status = 0;
> +		dev_dbg(udc_dev(dum), "set_address = %d\n",
> +				w_value);
> +		ret_val = 0;
> +		break;
> +	case USB_REQ_SET_FEATURE:
> +		if (setup.bRequestType == Dev_Request) {
> +			ret_val = 0;
> +			switch (w_value) {
> +			case USB_DEVICE_REMOTE_WAKEUP:
> +				break;
> +			case USB_DEVICE_B_HNP_ENABLE:
> +				dum->gadget.b_hnp_enable = 1;
> +				break;
> +			case USB_DEVICE_A_HNP_SUPPORT:
> +				dum->gadget.a_hnp_support = 1;
> +				break;
> +			case USB_DEVICE_A_ALT_HNP_SUPPORT:
> +				dum->gadget.a_alt_hnp_support = 1;
> +				break;
> +			case USB_DEVICE_U1_ENABLE:
> +				if (dum->gadget.speed == USB_SPEED_SUPER)
> +					w_value = USB_DEV_STAT_U1_ENABLED;
> +				else
> +					ret_val = -EOPNOTSUPP;
> +				break;
> +			case USB_DEVICE_U2_ENABLE:
> +				if (dum->gadget.speed == USB_SPEED_SUPER)
> +					w_value = USB_DEV_STAT_U2_ENABLED;
> +				else
> +					ret_val = -EOPNOTSUPP;
> +				break;
> +			case USB_DEVICE_LTM_ENABLE:
> +				if (dum->gadget.speed == USB_SPEED_SUPER)
> +					w_value = USB_DEV_STAT_LTM_ENABLED;
> +				else
> +					ret_val = -EOPNOTSUPP;
> +				break;

Where did these last three cases come from?  They aren't in the current
version of dummy-hcd.  The same is true for the cases under
USB_REQ_CLEAR_FEATURE.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/