Re: [PATCH v11 6/7] usb:dummy_hcd: use the shared_hcd infrustructure
From: Alan Stern <stern@rowland.harvard.edu>
Date: 2011-05-19 18:48:18
Also in:
lkml
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 <redacted>
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.
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
+ 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