> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, January 13, 2011 12:32 AM > To: Xu, Andiry > Cc: gregkh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > dbrownell@xxxxxxxxxxxxxxxxxxxxx; Alan Stern > Subject: Re: [PATCH 1/2] USB host: Move AMD PLL quirk to pci-quirks.c > > On Wed, Jan 12, 2011 at 04:25:45PM +0800, Xu, Andiry wrote: > > > > +static struct pci_dev *amd_nb_dev; > > > > +static struct pci_dev *amd_smbus_dev; > > > > +static int amd_nb_type; > > > > +static int amd_sb_type; > > > > +static int probe_count; > > > > +static DEFINE_SPINLOCK(amd_lock); > > > > + > > > > +int amd_find_chipset_info(void) > > > > +{ > > > > + u8 rev = 0; > > > > + static int ret; > > > > + unsigned long flags; > ... > > > > + > > > > + ret = 1; > > > > + printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n"); > > > > + > > > > + return ret; > > > > > > Why not just say "return 1;" here? > > > > > > > ret is a static variable. The logic of this function is: > > > > The first caller probes the whole chipset info, and set ret depends on > > the probe result; The following callers just increase probe_count and > > return ret, save the cost to probe again. > > > > So cannot just return 1 here. I'll modify the function that the first > > caller should hold the spinlock until it sets the ret value. > > Then please do not call it ret. Make it have a descriptive name, > because when I see ret I think "this is a temporary variable used to > store the return value", not "this is a global state variable". > > I think using static variables in this way is discouraged in the kernel. > Is there no structure you can stick this information in that can be > shared across all host controllers? Something in the USB core perhaps? > Or why not combine amd_nb_type, amd_sb_type, probe_count, amd_nb_dev, > amd_smbus_dev, and this new variable into one structure called > "amd_chipset" or something? > Thanks for the comment. I'll rename the global variable and combine all global variables into a struct. Thanks, Andiry -- 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
