Re: [PATCH] Add i2c-viapro.c

[Greg KH <greg@kroah.com>]

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