Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

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

On Wed, Jun 13, 2012 at 12:30:38PM -0700, Greg KH wrote:
> On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> > +
> > +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > +					wIndex + 1))
> 
> Why +1?  If you have to do this everywhere, then do it only in the
> function, so you can be 0 based properly.
> 
> Also, minor coding style nit, please rewrite as:
> 			if (usb_acpi_power_manageable(hcd->self.root_hub,
> 						      wIndex + 1))

This is an arbitrarily applied rule, and I don't follow it in the xHCI
driver.  The code indentation should be left at the default indentation
in the xHCI driver.  Let's keep the code style in the driver consistent,
please.

I don't understand why people want to move the trailing arguments to
align with the function parenthesis.  I can see that it might add to
readability for some people, but it's just more work on both the
developer's and maintainer's side.

Checkpatch will complain about mixed tabs and spaces, so that's more
work for me to edit my git pre-commit hook when the checkpatch error
stops git-am.  Plus the developer has to actually pause and think about
indentation, rather than letting their editor take care of it.

I'm not even sure when you would want the alignment rule applied.  In
all function calls with trailing arguments?  That seems really
excessive.

> Or even better yet, use a temp variable for the value returned and then
> check that, it's clearer and easier to read, right?

A temp variable would be fine until that function can just be
refactored.  The case statements should really just be broken out into
separate functions.

Sarah Sharp