lkml.org 
[lkml]   [2011]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v11 6/7] usb:dummy_hcd: use the shared_hcd infrustructure
    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@codeaurora.org>

    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



    \
     
     \ /
      Last update: 2011-05-19 20:51    [W:0.028 / U:0.088 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site