Messages in this thread | | | Date | Fri, 11 Apr 2003 12:54:15 -0700 | From | Greg KH <> | Subject | Re: [PATCH] Add i2c-viapro.c |
| |
On Fri, Apr 11, 2003 at 09:32:16PM +0200, Kronos wrote: > Hi, > the following patch (against 2.5.67) adds support for the SMBus bus on > VIA motherboard based of 82C596, 82C686 and 823x. It is needed, for > example, for the eeprom driver and for w83781d. I've tested it on a 8233 > chipset. Comments and feedback are welcome.
Looks good, with a few minor things that I can see:
> +/* Detect whether a compatible device can be found, and initialize it. */ > +int vt596_setup(void)
This function should take a struct pci_dev * as a paramater, that way we don't have to do:
> + /* First check whether we can access PCI at all */ > + if (pci_present() == 0) > + return(-ENODEV); > + > + /* Look for a supported device/function */ > + do { > + if((VT596_dev = pci_find_device(PCI_VENDOR_ID_VIA, num->dev, > + VT596_dev))) > + break; > + } while ((++num)->dev); > + > + if (VT596_dev == NULL) > + return(-ENODEV); > + dev_info("Found Via %s device\n", num->name);
All of this is not necessary, as the pci core will give is a proper pointer to a device that is in the pci device list that is passed to it.
> +#ifdef DEBUG > + if ((temp & 0x0E) == 8) > + dev_info(&VT596_dev->dev, "using Interrupt 9 for SMBus.\n"); > + else if ((temp & 0x0E) == 0) > + dev_info(&VT596_dev->dev, "using Interrupt SMI# for SMBus.\n"); > + else > + dev_warn(&VT596_dev->dev, "Illegal Interrupt configuration " > + "(or code out of date)!\n"); > + > + pci_read_config_byte(VT596_dev, SMBREV, &temp); > + dev_info(&VT596_dev->dev, "SMBREV = 0x%X\n", temp); > + dev_info(&VT596_dev->dev, "VT596_smba = 0x%X\n", vt596_smba); > +#endif /* DEBUG */
You can drop the #ifdef and change the dev_info() and dev_warn() calls here to dev_dbg().
> +static int __devinit vt596_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + int retval; > + > + retval = vt596_setup();
Here's where we can just pass the dev paramater to the function.
Other than those minor things, it looks very good. Almost ready to add to the tree.
thanks,
greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |