Re: [PATCH 2/2] i2c-i801: Add PCI idents for Sandy Bridge SMBus controllers

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

On Sat, 30 Oct 2010 19:15:50 -0400, David Woodhouse wrote:
> On Sat, 2010-10-30 at 18:36 +0200, Jean Delvare wrote:
> > As a side note, I really don't get the point of using 4 different PCI
> > device IDs for exactly the same device. Intel should really start to
> > worry about their numbering space if they keep attributing IDs when
> > they don't need to. 16 bit is fast to exhaust...
> 
> Don't we keep buying companies that have their own PCI vendor ID? :)
> 
> I certainly agree about the 0x1d7[012] IDs -- those seem fairly
> gratuitous. The main one (0x1d22) lacks slave mode though, so it is
> different from the others. Not that we care right now.
> 
> > > 
> > >     Features supported by this driver:
> > >     Software PEC                     no
> > > @@ -127,6 +131,11 @@
> > >   				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
> > >   				 SMBHSTSTS_INTR)
> > > 
> > > +#define PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_PCH	0x1d22
> > > +#define PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA1	0x1d70
> > > +#define PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA2	0x1d71
> > > +#define PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA3	0x1d72
> > > +
> > 
> > These should go to pci_ids.h together with all other similar defines.
> 
>  *      Do not add new entries to this file unless the definitions
>  *      are shared between multiple drivers.

I get the idea, but then why don't we move all other IDs back to
i2c-i801.c as well? It seems odd to have some of them in pci_ids.h and
the rest in i2c-i801.c.

Not something for this patch though, I agree.

> > >   struct i801_priv {
> > >   	struct i2c_adapter adapter;
> > >   	unsigned long smba;
> > > @@ -602,6 +611,10 @@ static const struct pci_device_id i801_ids[] = {
> > >   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH10_5) },
> > >   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5_3400_SERIES_SMBUS) },
> > >   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_PCH) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA1) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA2) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SANDYBRIDGE_SMBUS_EVA3) },
> > >   	{ 0, }
> > >   };
> > > 
> > 
> > You also have to list the new device in drivers/i2c/busses/Kconfig and
> > Documentation/i2c/busses/i2c-i801.
> 
> Will update.

Thanks,
-- 
Jean Delvare