Re: [PATCH v11 6/7] usb:dummy_hcd: use the shared_hcd infrustructure

[Alan Stern]

On Thu, 19 May 2011, Tatyana Brokhman wrote:

> This patch is a preparation for adding SuperSpeed support to dummy hcd.
> It takes the master side fields out of the struct dummy to a separate
> structure. The init process was also modified to resemble the way it is
> done by xHCI.
> 
> Signed-off-by: Tatyana Brokhman <tlinder@xxxxxxxxxxxxxx>

I have not checked this in detail, but right off the bat there are a 
few stylistic issues.

BTW, splitting this up into two patches as Felipe suggested was a very 
good idea.

> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -152,6 +152,24 @@ enum dummy_rh_state {
>  	DUMMY_RH_RUNNING
>  };
>  
> +struct dummy_hcd {
> +	struct usb_hcd *hcd;

What is this pointer for?  It doesn't get used by the 
dummy_hcd_to_hcd() routine, which means it shouldn't get used at all.

> +
> +	struct dummy			*dum;
> +	enum dummy_rh_state		rh_state;
> +	struct timer_list		timer;
> +	u32				port_status;
> +	u32				old_status;
> +	unsigned			resuming:1;

It's silly to have a bitfield in isolation like this -- you end up 
wasting a bunch of space.  Move this next to the other bitfields.

> +	unsigned long			re_timeout;
> +
> +	struct usb_device		*udev;
> +	struct list_head		urbp_list;
> +
> +	unsigned			active:1;
> +	unsigned			old_active:1;
> +};
> +
>  struct dummy {
>  	spinlock_t			lock;
>  
> @@ -167,36 +185,26 @@ struct dummy {
>  	u16				devstatus;
>  	unsigned			udc_suspended:1;
>  	unsigned			pullup:1;
> -	unsigned			active:1;
> -	unsigned			old_active:1;
>  
>  	/*
>  	 * MASTER/HOST side support
>  	 */
> -	enum dummy_rh_state		rh_state;
> -	struct timer_list		timer;
> -	u32				port_status;
> -	u32				old_status;
> -	unsigned			resuming:1;
> -	unsigned long			re_timeout;
> -
> -	struct usb_device		*udev;
> -	struct list_head		urbp_list;
> +	struct dummy_hcd *hs_hcd;

You should use tabs to line up the columns with the existing entries.  
Doesn't this look strange to you -- a single item in the middle of the 
line by itself like that?

>  };
>  
> -static inline struct dummy *hcd_to_dummy (struct usb_hcd *hcd)
> +static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
>  {
> -	return (struct dummy *) (hcd->hcd_priv);
> +	return (struct dummy_hcd *) (hcd->hcd_priv);
>  }
>  
> -static inline struct usb_hcd *dummy_to_hcd (struct dummy *dum)
> +static inline struct usb_hcd *dummy_hcd_to_hcd(struct dummy_hcd *dum)
>  {
>  	return container_of((void *) dum, struct usb_hcd, hcd_priv);
>  }

In several places you open-coded dummy_hcd_to_hcd().  Please change
them to use the inline routine instead.

> -static inline struct device *dummy_dev (struct dummy *dum)
> +static inline struct device *dummy_dev(struct dummy_hcd *dum)
>  {
> -	return dummy_to_hcd(dum)->self.controller;
> +	return dummy_hcd_to_hcd(dum)->self.controller;
>  }

You also open-coded this routine.  It doesn't matter so much, but for 
the sake of style you should use this inline routine too.

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/