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

[Lan Tianyu <tianyu.lan@intel.com>]

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@intel.com>
>>>> ---
>>>>   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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html