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
