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

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 14, 2012 at 03:22:42PM +0800, Lan Tianyu wrote:
> On 2012年06月14日 03:30, Greg KH wrote:
> >On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> >>On our developping machine, bios can provide usb port's  power control via
> >>acpi. This patch is to provide usb port's power control way through setting
> >>or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
> >>and usb_acpi_set_power_state(). The first one is used to find whether the
> >>usb port has acpi power resource and the second is to set the power state.
> >>They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
> >>feature requests are processed.
> >>
> >>Signed-off-by: Lan Tianyu<tianyu.lan@xxxxxxxxx>
> >>---
> >>  drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
> >>  drivers/usb/host/xhci-hub.c |   10 ++++++++++
> >>  include/linux/usb.h         |   10 ++++++++++
> >>  3 files changed, 48 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> >>index 82c90d0..e95f26f 100644
> >>--- a/drivers/usb/core/usb-acpi.c
> >>+++ b/drivers/usb/core/usb-acpi.c
> >>@@ -19,6 +19,34 @@
> >>
> >>  #include "usb.h"
> >>
> >>+bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
> >>+{
> >>+	acpi_handle port_handle;
> >>+
> >>+	port_handle = usb_get_hub_port_acpi_handle(hdev,
> >>+		port1);
> >>+	return port_handle ? acpi_bus_power_manageable(port_handle) : false;
> >
> >Ick, I _really_ hate the ? : usage in C, please use real if statements
> >so that everyone can read and understand them easier.  You do that a lot
> >here, please fix them all.
> >
> Ok. But in some places, for example
> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
> 		port1, enable ? "enable" : "disable");
> try to print two result. ?: is more convenient. If I use "if" statement,
> that will be
> if (enable)
> 	result = "enable";
> else
> 	result = "disable";
> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
> 		port1, result);
> 
> This just looks a little complex.

I understand that, which is why I didn't complain about that.  But even
then, you could just change the line to:
	dev_dbg(&hdev->dev, "The power of hub port %d was set to %d\n", port1, enable);
and you would be able to tell what is going on.


> >>+}
> >>+EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
> >>+
> >>+int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
> >>+{
> >>+	acpi_handle port_handle;
> >>+	unsigned char state;
> >>+	int error = -EINVAL;
> >>+
> >>+	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
> >>+		port1);
> >>+	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
> >>+	error = acpi_bus_set_power(port_handle, state);
> >
> >You forgot to check port_handle here.
> >
> >Why not call usb_acpi_power_manageable() to ensure that you can do this?
> 
> In my code, usb_acpi_power_manageable() is invoked before
> usb_acpi_set_power_state().

Are you sure that all future callers will always do this?

> Do you mean I should call usb_acpi_power_manageable() in the
> usb_acpi_set_power_state()?

Maybe, if it makes sense to.  But don't fail, like you currently do, if
someone doesn't do that, as you aren't telling the caller that they are
required to do so.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html