On Fri, Sep 06, 2013 at 11:37:28PM +0800, Greg Kroah-Hartman wrote:
> 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.
>
Hi Greg,
I've got the approvement to open the chipset platform names. :)
So I wil use enumerated type or #define to describe the platforms. We
always use SMBus device id and revision to distinguish different
chipset platforms.
> > }
> >
> > - if (info.sb_type == 0) {
> > + if (info.sb_type == 0 || info.sb_type == 4) {
>
> Why type 4 here?
>
Because type 4 is a new chipset and usb_amd_find_chipset_info is only
for AMD PLL quirk at old platforms. So it should go out the function
like type 0 (undefined type).
> > 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?
>
OK, will do it.
> > + 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.
>
Actually, I have a little confused. Assume that we write a core level
function, and it would be called in host controller level such as
xhci-*, ehci-*. Should I split the codes from core and host in two
pathes or write one patch included core and host level updates?
Thanks,
Rui
--
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
