Re: [PATCH] Add i2c-viapro.c

Greg KH (greg@kroah.com)
Fri, 11 Apr 2003 12:54:15 -0700


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/