* Re: [PATCH v2] I2C: Add support for new AMD SMBus devices
2009-08-11 1:49 [PATCH v2] I2C: Add support for new AMD SMBus devices Crane Cai
@ 2009-09-09 8:12 ` Jean Delvare
2009-09-09 9:10 ` Jean Delvare
1 sibling, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2009-09-09 8:12 UTC (permalink / raw)
To: Crane Cai
Cc: linux-kernel-gepDB1WL6pn9qxiX1TGQuw,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
Hi Crane,
Sorry for the late reply.
On Tue, 11 Aug 2009 09:49:21 +0800, Crane Cai wrote:
> AMD newer SMBus device uses the piix4 driver, and device is added by vendor ID
> and class code. This method will include AMD old SMBus controller in, and need
> to filter it out.
This is better, however I still have concerns:
* Your device ID comparison is fragile. Are you absolutely certain that
you know _all_ the SMBus devices that were released by AMD in the
past? If anything, I'd rather have a single comparison on the first
device ID that _will_ be supported by the i2c-piix4 driver.
* Are you absolutely certain that AMD will never ever release an
incompatible SMBus device in the future? I doubt it. In the past,
various vendors have changed their implementations. Intel changed
twice (i2c-piix4 -> i2c-i801 -> i2c-isch), AMD once already
(i2c-amd756 -> i2c-amd8111), VIA once (i2c-via -> i2c-viapro), nVidia
once (i2c-amd756 -> i2c-nforce2), etc. If history repeats itself (and
it often does), I wouldn't be surprised to see future AMD SMBus chips
not compatible with the i2c-piix4 driver, while with your patch
applied, the driver would claim to support them. Binding a driver to
an unsupported chip can have unpredictable, potentially bad
consequences, and I'd rather have the driver not bind immediately to
new supported devices than bind to unsupported ones.
* With your patch, the i2c-piix4 driver would load automatically on
systems which do not need it (all old AMD-based PC systems.) While a
minor issue, this is still a waste of boot time and memory for these
users. I am not aware of any precedent of this.
So, all in all, I'd rather simply add the IDs of new supported devices
as they are released, rather than implement this broad matching
mechanism.
If AMD is bored with adding new device IDs to Linux drivers all the
time (and I certainly am) then the cleanest solution is to simply not
allocate new device IDs for fully compatible (if not even completely
identical silicon-wise) devices. For example, AMD could allocate a
single PCI device ID for their PIIX4-compatible SMBus chips, we name it
PCI_DEVICE_ID_AMD_SMBUS_PIIX4 for example, and all future compatible
chips use this device ID. You can use the revision ID field if you want
to be able to differentiate between the various incarnations.
>
> Signed-off-by: Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-piix4.c | 17 +++++++++++++++--
> 1 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 0249a7d..2539282 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -232,6 +232,14 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> unsigned short smba_idx = 0xcd6;
> u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
>
> + /* Filter AMD old SMBus controller out */
> + if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD &&
> + (PIIX4_dev->device <= PCI_DEVICE_ID_AMD_8111_SMBUS &&
> + PIIX4_dev->device >= PCI_DEVICE_ID_AMD_VIPER_740B)) {
> + dev_info(&PIIX4_dev->dev, "Does not support this chip.\n");
> + return -ENODEV;
> + }
> +
> /* SB800 SMBus does not support forcing address */
> if (force || force_addr) {
> dev_err(&PIIX4_dev->dev, "SB800 SMBus does not support "
> @@ -479,6 +487,10 @@ static struct pci_device_id piix4_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP300_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS) },
> + /* AMD Generic, PCI class code and Vendor ID for SMBus */
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_ANY_ID),
> + .class = PCI_CLASS_SERIAL_SMBUS << 8,
> + .class_mask = 0xffffff },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
> PCI_DEVICE_ID_SERVERWORKS_OSB4) },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
> @@ -499,9 +511,10 @@ static int __devinit piix4_probe(struct pci_dev *dev,
> {
> int retval;
>
> - if ((dev->vendor == PCI_VENDOR_ID_ATI) &&
> + if (((dev->vendor == PCI_VENDOR_ID_ATI) &&
> (dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS) &&
> - (dev->revision >= 0x40))
> + (dev->revision >= 0x40)) ||
> + dev->vendor == PCI_VENDOR_ID_AMD)
> /* base address location etc changed in SB800 */
> retval = piix4_setup_sb800(dev, id);
> else
--
Jean Delvare
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] I2C: Add support for new AMD SMBus devices
2009-08-11 1:49 [PATCH v2] I2C: Add support for new AMD SMBus devices Crane Cai
2009-09-09 8:12 ` Jean Delvare
@ 2009-09-09 9:10 ` Jean Delvare
[not found] ` <20090909111026.21e85f54-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
1 sibling, 1 reply; 4+ messages in thread
From: Jean Delvare @ 2009-09-09 9:10 UTC (permalink / raw)
To: Crane Cai
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
[Resending with fixed LKML address.]
Hi Crane,
Sorry for the late reply.
On Tue, 11 Aug 2009 09:49:21 +0800, Crane Cai wrote:
> AMD newer SMBus device uses the piix4 driver, and device is added by vendor ID
> and class code. This method will include AMD old SMBus controller in, and need
> to filter it out.
This is better, however I still have concerns:
* Your device ID comparison is fragile. Are you absolutely certain that
you know _all_ the SMBus devices that were released by AMD in the
past? If anything, I'd rather have a single comparison on the first
device ID that _will_ be supported by the i2c-piix4 driver.
* Are you absolutely certain that AMD will never ever release an
incompatible SMBus device in the future? I doubt it. In the past,
various vendors have changed their implementations. Intel changed
twice (i2c-piix4 -> i2c-i801 -> i2c-isch), AMD once already
(i2c-amd756 -> i2c-amd8111), VIA once (i2c-via -> i2c-viapro), nVidia
once (i2c-amd756 -> i2c-nforce2), etc. If history repeats itself (and
it often does), I wouldn't be surprised to see future AMD SMBus chips
not compatible with the i2c-piix4 driver, while with your patch
applied, the driver would claim to support them. Binding a driver to
an unsupported chip can have unpredictable, potentially bad
consequences, and I'd rather have the driver not bind immediately to
new supported devices than bind to unsupported ones.
* With your patch, the i2c-piix4 driver would load automatically on
systems which do not need it (all old AMD-based PC systems.) While a
minor issue, this is still a waste of boot time and memory for these
users. I am not aware of any precedent of this.
So, all in all, I'd rather simply add the IDs of new supported devices
as they are released, rather than implement this broad matching
mechanism.
If AMD is bored with adding new device IDs to Linux drivers all the
time (and I certainly am) then the cleanest solution is to simply not
allocate new device IDs for fully compatible (if not even completely
identical silicon-wise) devices. For example, AMD could allocate a
single PCI device ID for their PIIX4-compatible SMBus chips, we name it
PCI_DEVICE_ID_AMD_SMBUS_PIIX4 for example, and all future compatible
chips use this device ID. You can use the revision ID field if you want
to be able to differentiate between the various incarnations.
>
> Signed-off-by: Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-piix4.c | 17 +++++++++++++++--
> 1 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 0249a7d..2539282 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -232,6 +232,14 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> unsigned short smba_idx = 0xcd6;
> u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
>
> + /* Filter AMD old SMBus controller out */
> + if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD &&
> + (PIIX4_dev->device <= PCI_DEVICE_ID_AMD_8111_SMBUS &&
> + PIIX4_dev->device >= PCI_DEVICE_ID_AMD_VIPER_740B)) {
> + dev_info(&PIIX4_dev->dev, "Does not support this chip.\n");
> + return -ENODEV;
> + }
> +
> /* SB800 SMBus does not support forcing address */
> if (force || force_addr) {
> dev_err(&PIIX4_dev->dev, "SB800 SMBus does not support "
> @@ -479,6 +487,10 @@ static struct pci_device_id piix4_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP300_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS) },
> + /* AMD Generic, PCI class code and Vendor ID for SMBus */
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_ANY_ID),
> + .class = PCI_CLASS_SERIAL_SMBUS << 8,
> + .class_mask = 0xffffff },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
> PCI_DEVICE_ID_SERVERWORKS_OSB4) },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
> @@ -499,9 +511,10 @@ static int __devinit piix4_probe(struct pci_dev *dev,
> {
> int retval;
>
> - if ((dev->vendor == PCI_VENDOR_ID_ATI) &&
> + if (((dev->vendor == PCI_VENDOR_ID_ATI) &&
> (dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS) &&
> - (dev->revision >= 0x40))
> + (dev->revision >= 0x40)) ||
> + dev->vendor == PCI_VENDOR_ID_AMD)
> /* base address location etc changed in SB800 */
> retval = piix4_setup_sb800(dev, id);
> else
--
Jean Delvare
^ permalink raw reply [flat|nested] 4+ messages in thread