On Tue, Jan 22, 2013 at 11:32:13PM +0800, Lan Tianyu wrote:
> On 2013/1/22 5:30, Greg KH wrote:
> >On Mon, Jan 21, 2013 at 10:18:04PM +0800, Lan Tianyu wrote:
> >>@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int portstatus)
> >> }
> >>
> >> /* Note that hdev or one of its children must be locked! */
> >>-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
> >>+struct usb_hub *hdev_to_hub(struct usb_device *hdev)
> >
> >This needs a better name now that this is a global function. No one
> >knows what a "hdev" is.
> Actually the routine will only be used in the
> driver/usb/core/(hub.c, port.c). Port also should be the part of
> hub, right? Moving port related code to port.c is to make code more
> clear. I am not sure whether we should rename it.
It is now a global symbol in the kernel name space, so yes, it should be
renamed.
> If we renamed it, how about "usb_hub_to_struct_hub"? \
> Should we do rename operation in a separate patch since
> hdev_to_hub() is called many places in the hub.c?
That's fine with me.
> >> {
> >> if (!hdev || !hdev->actconfig || !hdev->maxchild)
> >> return NULL;
> >>@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int feature)
> >> /*
> >> * USB 2.0 spec Section 11.24.2.2
> >> */
> >>-static int clear_port_feature(struct usb_device *hdev, int port1, int feature)
> >>+int clear_port_feature(struct usb_device *hdev, int port1, int feature)
> >
> >This is now a global function, please put usb_ infront of it.
> >
> Same above?
Yes.
> >>-static int hub_port_debounce(struct usb_hub *hub, int port1)
> >>+int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected)
> >
> >I hate boolean flags in functions that do different things depending on
> >the value. Can you just make two different functions here, and have
> >them call this private one with the proper flag set?
> Sorry. I am not very sure I understand correctly.
> Do you mean to create two wraps which call hub_port_debounce()?
> Just like
> hub_port_debounce_be_counted()
> {
> hub_port_debounce(...,..., true);
> }
>
> hub_port_debounce_be_counted()
> {
> hub_port_debounce(...,..., false);
> }
If you name the functions differently, yes :)
> >>@@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, int port1)
> >>
> >> if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
> >> (portstatus & USB_PORT_STAT_CONNECTION) == connection) {
> >>- stable_time += HUB_DEBOUNCE_STEP;
> >>+ if (!must_be_connected || (connection
> >>+ == USB_PORT_STAT_CONNECTION))
> >
> >Please do:
> > if (!must_be_connected ||
> > (connection == USB_PORT_STAT_CONNECTION))
> >
> >instead, that makes it much more readable.
> Oh. I originally do the same thing. But there is following code
> which requires this line to be more indention. This will cause this
> line over 80 characters. So I had to break this line.
The above line, as written is under 80 characters, so I don't understand
the issue.
> I finally find that if I just do following, it also can work.
> if (!must_be_connected || connection)
> stable_time += HUB_DEBOUNCE_STEP;
> Does this make sense? Since connection only will be assigned to the
> result of "portstatus & USB_PORT_STAT_CONNECTION".
Connection is an enumerated type, please be explicit when testing it.
thanks,
greg k-h
--
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
