On 2013/1/23 0:02, Greg KH wrote:
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.
If I do following, scripts/checkpatch.pl will complain over 80 characters.
@@ -3767,7 +3814,9 @@
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))
+ stable_time += HUB_DEBOUNCE_STEP;
if (stable_time >= HUB_DEBOUNCE_STABLE)
break;
} else {
WARNING: line over 80 characters
#203: FILE: drivers/usb/core/hub.c:3818:
+ (connection == USB_PORT_STAT_CONNECTION))
If I use (connection & USB_PORT_STAT_CONNECTION)), that will be ok.
I am curious about that checkpatch.pl considers a space or tab as a character?
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
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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
