Hi Greg:
Great thanks for your review. I learn a lot from your comments which I
have not noticed before.
On 2013年01月22日 05:20, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:01PM +0800, Lan Tianyu wrote:
>> This patch is to register usb port's acpi power resources. Create
>> link between usb port device and its acpi power resource.
>>
>> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>> ---
>> drivers/usb/core/port.c | 3 +++
>> drivers/usb/core/usb-acpi.c | 20 ++++++++++++++++++++
>> drivers/usb/core/usb.h | 6 ++++++
>> 3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index fe5959f..4dfa254 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -66,6 +66,7 @@ static void usb_port_device_release(struct device *dev)
>> {
>> struct usb_port *port_dev = to_usb_port(dev);
>>
>> + usb_acpi_unregister_power_resources(dev);
>> kfree(port_dev);
>> }
>>
>> @@ -95,6 +96,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>> if (retval)
>> goto error_register;
>>
>> + usb_acpi_register_power_resources(&port_dev->dev);
>> +
>> return 0;
>>
>> error_register:
>> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
>> index cef4252..558ab01 100644
>> --- a/drivers/usb/core/usb-acpi.c
>> +++ b/drivers/usb/core/usb-acpi.c
>> @@ -216,6 +216,26 @@ static struct acpi_bus_type usb_acpi_bus = {
>> .find_device = usb_acpi_find_device,
>> };
>>
>> +int usb_acpi_register_power_resources(struct device *dev)
>> +{
>> + acpi_handle port_handle = DEVICE_ACPI_HANDLE(dev);
>> +
>> + if (!port_handle)
>> + return -ENODEV;
>> +
>> + if (acpi_power_resource_register_device(dev, port_handle) < 0)
>> + return -ENODEV;
>> + return 0;
>> +}
>
> Why return a value if you never check it? Either properly handle the
> error in the place you call this function, or don't have a return value.
>
> I'd prefer you to handle the error properly.
Not all system will provide acpi power resouces for usb port and this
will not damage usb device function. So I think I should produce some
warning if the return value is not ENODEV.
>
> thanks,
>
> greg k-h
>
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
