On Fri, Sep 06, 2013 at 06:24:42PM +0800, Huang Rui wrote:
> It add a remote wakeup pci quirk for some special AMD platforms. This
> quirk filters southbridge revision which is 0x39 or 0x3a.
>
> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> ---
> drivers/usb/host/pci-quirks.c | 14 +++++++++++++-
> drivers/usb/host/pci-quirks.h | 1 +
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 2c76ef1..d440e73 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -140,9 +140,11 @@ int usb_amd_find_chipset_info(void)
> rev = info.smbus_dev->revision;
> if (rev >= 0x11 && rev <= 0x18)
> info.sb_type = 2;
> + if (rev >= 0x39 && rev <= 0x3a)
> + info.sb_type = 4;
You are adding another "type" here, yet we have no information on what
these "types" mean. Can you please use an enumerated type, or a #define
at the least, for all of these so we can start to tell them apart?
Also, what does 39 and 3a mean? Please use a #define with a name we can
understand so that others can know what this means.
> }
>
> - if (info.sb_type == 0) {
> + if (info.sb_type == 0 || info.sb_type == 4) {
Why type 4 here?
> if (info.smbus_dev) {
> pci_dev_put(info.smbus_dev);
> info.smbus_dev = NULL;
> @@ -197,6 +199,16 @@ commit:
> }
> EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
>
> +int usb_amd_remote_wakeup_quirk(void)
> +{
> + if (amd_chipset.sb_type != 4)
> + return 0;
> +
> + printk(KERN_DEBUG "QUIRK: Enable AMD remote wakeup fix\n");
dev_dbg() please?
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(usb_amd_remote_wakeup_quirk);
Nothing calls this function, so why have you added it here?
The way you have broken up the patches is a bit odd. You are creating
functions that aren't called yet, but do not describe this in the
changelog information, so people reviewing the patch would just assume
they are not needed at all (hint, like I just did...)
Please provide a better description, and justification, if you create a
new function, otherwise we will assume it is not needed at all.
thanks,
greg k-h
--
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
