On 2012年06月14日 23:44, Greg KH wrote:
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.
Ok. I get it.
+}
+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.
How about to add kernel-doc to make future user notice it? On the other side,
I check the acpi_bus_set_power and if the port didn't have acpi power resource,
it would return error and no harm.
thanks,
greg k-h
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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
