Re: [PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware

[Sarah Sharp <sarah.a.sharp@linux.intel.com>]

On Wed, Aug 01, 2012 at 04:46:27PM -0500, Alexis R. Cortes wrote:
> Hi Sarah,
> 
> Sure!! I'll update the patch's description and will send another patch in a
> few moments.
> 
> As an additional comment, I ran the 'checkpatch.pl' script and verified
> there were no errors before submitting the patch. I only got a few Warnings
> related to the line limits. 

Can you break the code up a bit so the line limit warnings don't appear?
The long line warnings are really indicative of a deeper problem,
usually that the parent function has gotten much too long, and should be
broken into smaller functions.

The only exception to the long line rule (and one that checkpatch
doesn't usually complain about) is that we don't break up lines in the
middle of strings.  It makes searching for the strings later much
harder.  So either leave the really long string line, or break it up
into two xhci_dbg statements.

In general, warnings from checkpatch.pl stop my patch application and
submission work flow, so I don't accept patches with warnings.

For instance:

			if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
				if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1)) {
					if ((temp & PORT_PLS_MASK) == XDEV_U0) {
						xhci->port_status_u0 |= 1 << wIndex;
						if (xhci->port_status_u0 == ((1 << xhci->num_usb3_ports)-1)) {
							del_timer_sync(&xhci->comp_mode_recovery_timer);
							xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted. "
                                                                       "All USB3 ports have entered U0 at least once.\n");
                                               }
                                       }
                               }
                       }

Could be broken out into a new function, like so:

void xhci_del_comp_mod_timer(struct xhci_hcd *xhci)
{
	unsigned int all_ports_seen_u0 = ((1 << xhci->num_usb3_ports)-1));
	boolean this_port_in_u0 = ((temp & PORT_PLS_MASK) == XDEV_U0);

	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
		return;

	if (xhci->port_status_u0 != all_ports_seen_u0 && this_port_in_u0) {
		xhci->port_status_u0 |= 1 << wIndex;
		if (xhci->port_status_u0 == all_ports_seen_u0) {
			del_timer_sync(&xhci->comp_mode_recovery_timer);
			xhci_dbg(xhci, "All USB3 ports have entered U0 at least once.\n");
			xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted.\n");
		}
	}
}

Then you can call that new function in the ugly long indent-heavy function
in xhci-hub.c.

Sarah Sharp