| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120805140454.3e7a9606@endymion.delvare>
Date: Sun, 5 Aug 2012 14:04:54 +0200
From: Jean Delvare <khali@...ux-fr.org>
To: Amaury Decrême <amaury.decreme@...il.com>
Cc: ben-linux@...ff.org, w.sang@...gutronix.de, rob@...dley.net,
jeffrey.t.kirsher@...el.com, akpm@...ux-foundation.org,
davem@...emloft.net, gregkh@...uxfoundation.org, joe@...ches.com,
ralf@...ux-mips.org, dirk.brandewie@...il.com,
jayachandranc@...logicmicro.com, linux-i2c@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
mhoffman@...htlink.com, amalysh@....de,
李志村 (nelson) <nelson@....com>
Subject: Re: [PATCH v2 0/2] I2C: SIS964: Bus driver
Salut Amaury,
On Sat, 4 Aug 2012 00:38:29 +0000, Amaury Decrême wrote:
> > There's nothing confusing, drivers supporting several devices are
> > legion. If the devices are really almost compatible, reusing an
> > existing driver is the way to go.
>
> With that in mind, here is an alpha preview of what the patch will
> look like if SIS964 support is added in i2c-sis630.
If that's all that is needed to get the SIS964 supported, then we
definitely don't want a separate driver.
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 5d6723b..861d58b 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -33,6 +33,8 @@
> Fixed logical error by restoring of Host Master Clock
> 31.07.2003
> Added block data read/write support.
> + 03.08.2012
> + Added support of SiS964 - Amaury Decrême <amaury.decreme@...il.com>
> */
>
> /*
> @@ -41,6 +43,7 @@
> Supports:
> SIS 630
> SIS 730
> + SIS 964
>
> Note: we assume there can only be one device, with one SMBus interface.
> */
> @@ -55,22 +58,22 @@
> #include <linux/acpi.h>
> #include <linux/io.h>
>
> +/* SIS964 id, defined here as we are the only file using it */
> +#define PCI_DEVICE_ID_SI_964 0x0964
> +
> /* SIS630 SMBus registers */
> -#define SMB_STS 0x80 /* status */
> -#define SMB_EN 0x81 /* status enable */
> -#define SMB_CNT 0x82
> -#define SMBHOST_CNT 0x83
> -#define SMB_ADDR 0x84
> -#define SMB_CMD 0x85
> -#define SMB_PCOUNT 0x86 /* processed count */
> -#define SMB_COUNT 0x87
> -#define SMB_BYTE 0x88 /* ~0x8F data byte field */
> -#define SMBDEV_ADDR 0x90
> -#define SMB_DB0 0x91
> -#define SMB_DB1 0x92
> -#define SMB_SAA 0x93
> -
> -/* register count for request_region */
> +#define SMB_STS 0x00 + offset /* status */
> +#define SMB_CNT 0x02 + offset /* control */
> +#define SMBHOST_CNT 0x03 + offset /* host control */
> +#define SMB_ADDR 0x04 + offset /* address */
> +#define SMB_CMD 0x05 + offset /* command */
> +#define SMB_COUNT 0x07 + offset /* byte count */
> +#define SMB_BYTE 0x08 + offset /* ~0x8F data byte field */
> +#define SMB_SAA 0x13 + offset /* host slave
> alias address */
Your email client apparently folds long lines, you'll have to fix that
when you resend.
The above definitions are dangerous. At the very least you need
parentheses around. Better would be to pass the offset as a parameter.
Best would be to only define the constants and add the offset in
the calling code. There are only 8 locations so it should be easy.
> +
> +/* register count for request_region
> + * As we don't use SMB_PCOUNT 20 is ok for SiS630 and SiS964
> + */
> #define SIS630_SMB_IOREGION 20
>
> /* PCI address constants */
> @@ -107,9 +110,13 @@ static unsigned short acpi_base;
> static int supported[] = {
> PCI_DEVICE_ID_SI_630,
> PCI_DEVICE_ID_SI_730,
> + PCI_DEVICE_ID_SI_964,
> 0 /* terminates the list */
> };
>
> +/* SMB registers offset */
> +static int offset;
Please move this declaration close to acpi_base.
It would be more efficient to not introduce an offset variable but
rather smbus_base = acpi_base + offset. That way you save an addition
each time you access a register.
> +
> static inline u8 sis630_read(u8 reg)
> {
> return inb(acpi_base + reg);
> @@ -412,6 +419,10 @@ static int __devinit sis630_setup(struct pci_dev
> *sis630_dev)
> return -ENODEV;
> }
>
> + if (supported[i] == PCI_DEVICE_ID_SI_964)
> + offset = 0xE0;
> + else
> + offset = 0x80;
> /*
> Enable ACPI first , so we can accsess reg 74-75
> in acpi io space and read acpi base addr
> @@ -474,6 +485,7 @@ static struct i2c_adapter sis630_adapter = {
>
> static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
> { PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503) },
> + { PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_964) },
> { PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_LPC) },
> { 0, }
> };
> @@ -482,6 +494,7 @@ MODULE_DEVICE_TABLE (pci, sis630_ids);
>
> static int __devinit sis630_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> {
> + dev_dbg(&dev->dev, "salut");
Bien le bonjour à toi aussi :)
> if (sis630_setup(dev)) {
> dev_err(&dev->dev, "SIS630 comp. bus not detected,
> module not inserted.\n");
> return -ENODEV;
>
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists