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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
