i2c driver fixes for 2.6.0-test2

PATCH New Greg KH 3 participants 12 messages Aug 2, 2003 – May 19, 2005

Subsystems (2)

I2C subsystem host drivers I2C/smbus controller drivers for pc

lore

Share & tools

lore

Greg KH

author

2003-08-02 05:28 UTC #0 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

Hi,

Here are some i2c driver fixes for 2.6.0-test2. They include a number of minor i2c fixes and add a new i2c bus controller driver.

Please pull from: bk://kernel.bkbits.net/gregkh/linux/i2c-2.5

thanks,

greg k-h

 drivers/i2c/Kconfig              |    8 
 drivers/i2c/busses/Kconfig       |   17 +
 drivers/i2c/busses/Makefile      |    1 
 drivers/i2c/busses/i2c-ali1535.c |   20 -
 drivers/i2c/busses/i2c-ali15x3.c |   15 -
 drivers/i2c/busses/i2c-amd756.c  |   28 --
 drivers/i2c/busses/i2c-amd8111.c |    6 
 drivers/i2c/busses/i2c-i801.c    |   28 --
 drivers/i2c/busses/i2c-nforce2.c |  470 +++++++++++++++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-piix4.c   |   20 -
 drivers/i2c/busses/i2c-sis96x.c  |   18 -
 drivers/i2c/busses/i2c-viapro.c  |   18 -
 drivers/i2c/chips/via686a.c      |   20 -
 drivers/i2c/i2c-keywest.c        |   90 +------
 include/linux/i2c.h              |    7 
 15 files changed, 529 insertions(+), 237 deletions(-)

-----

<nikkne:hotpop.com>:
o I2C: fix Kconfig info

<patrick:dreker.de>:
o I2C: add ncforce2 i2c bus driver

Benjamin Herrenschmidt:
o I2C: timer clean up for i2c-keywest.c

Daniele Bellucci:
o I2C: fixed a little memory leak in i2c-ali15x3.c

Greg Kroah-Hartman:
o I2C: remove devinitdata marking from i2c-nforce2.c as it's wrong
o I2C: consolidate the i2c delay functions
o I2C: minor cleanups to the i2c-nforce2 driver

Jan Dittmer:
o I2C: convert via686a temp_* to milli degree celsius

Mark M. Hoffman

2003-08-14 05:14 UTC #2 lore raw

In reply to Mark M. Hoffman

This is a resend of a patch to the i2c-nforce2.c I2C bus driver. The start of the relevant thread was here:
http://archives.andrew.net.au/lm-sensors/msg03820.html

[comment]
This patch restores a line that was wrongly removed. There are also some trivial cleanups. It applies & compiles vs. 2.6.0-test3. It's untested (no hardware here).
[/comment]

drivers/i2c/busses/i2c-nforce2.c renamed from drivers/i2c/busses/i2c-nforce2.c.orig +5 ~~-24~~

@@ -51,10 +51,6 @@
 #ifndef PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS
 #define PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS   0x0064
 #endif
-/* TODO: sync with lm-sensors */
-#ifndef I2C_HW_SMBUS_NFORCE2
-#define I2C_HW_SMBUS_NFORCE2	0x0c
-#endif
 
 
 struct nforce2_smbus {

@@ -128,20 +124,10 @@
 	.name   	= "unset",
 };
 
-
-#if 0
-/* Internally used pause function */
-static void nforce2_do_pause(unsigned int amount)
-{
-	current->state = TASK_INTERRUPTIBLE;
-	schedule_timeout(amount);
-}
-#endif
-
 /* Return -1 on error. See smbus.h for more information */
-static s32 nforce2_access(struct i2c_adapter * adap, u16 addr, unsigned short flags,
-		char read_write, u8 command, int size,
-		union i2c_smbus_data * data)
+static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
+		unsigned short flags, char read_write,
+		u8 command, int size, union i2c_smbus_data * data)
 {
 	struct nforce2_smbus *smbus = adap->algo_data;
 	unsigned char protocol, pec, temp;

@@ -249,7 +235,7 @@
 
 #if 0
 	do {
-		nforce2_do_pause(1);
+		i2c_do_pause(1);
 		temp = inb_p(NVIDIA_SMB_STS);
 	} while (((temp & NVIDIA_SMB_STS_DONE) == 0) && (timeout++ < MAX_TIMEOUT));
 #endif

@@ -332,13 +318,8 @@
 			smbus->base, smbus->base+smbus->size-1, name);
 		return -1;
 	}
-/*
-	smbus->adapter.owner = THIS_MODULE;
-	smbus->adapter.id = I2C_ALGO_SMBUS | I2C_HW_SMBUS_NFORCE2;
-	smbus->adapter.algo = &smbus_algorithm;
-	smbus->adapter.algo_data = smbus;
-*/
 	smbus->adapter = nforce2_adapter;
+	smbus->adapter.algo_data = smbus;
 	smbus->adapter.dev.parent = &dev->dev;
 	snprintf(smbus->adapter.name, DEVICE_NAME_SIZE,
 		"SMBus nForce2 adapter at %04x", smbus->base);

Signature · 2 lines

Mark M. Hoffman
[email protected]

Greg KH

author

2003-08-14 21:16 UTC #3 lore raw

Quoted 9 lines · Mark M. Hoffman

This is a resend of a patch to the i2c-nforce2.c I2C bus driver. The start of the relevant thread was here:
http://archives.andrew.net.au/lm-sensors/msg03820.html
[comment]
This patch restores a line that was wrongly removed. There are also some trivial cleanups. It applies & compiles vs. 2.6.0-test3. It's untested (no hardware here).
[/comment]

Oops, sorry I forgot this one. I've now applied it and will send it on later to Linus.

greg k-h

Jean Delvare

2005-05-19 06:24 UTC #4 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

In reply to Greg KH

Greg,

Quoted 12 lines

<nikkne:hotpop.com>:
o I2C: fix Kconfig info
<patrick:dreker.de>:
o I2C: add ncforce2 i2c bus driver
Benjamin Herrenschmidt:
o I2C: timer clean up for i2c-keywest.c
Daniele Bellucci:
o I2C: fixed a little memory leak in i2c-ali15x3.c

Quoted 7 lines

Greg Kroah-Hartman:
o I2C: remove devinitdata marking from i2c-nforce2.c as it's wrong
o I2C: consolidate the i2c delay functions
o I2C: minor cleanups to the i2c-nforce2 driver
Jan Dittmer:
o I2C: convert via686a temp_* to milli degree celsius

Does any of these apply to our CVS repository?

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Greg KH

author

2005-05-19 06:24 UTC #5 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

In reply to Greg KH

Quoted 20 lines · Jean Delvare

Greg,
Quoted 17 lines
<nikkne:hotpop.com>:
o I2C: fix Kconfig info
<patrick:dreker.de>:
o I2C: add ncforce2 i2c bus driver
Benjamin Herrenschmidt:
o I2C: timer clean up for i2c-keywest.c
Daniele Bellucci:
o I2C: fixed a little memory leak in i2c-ali15x3.c
Greg Kroah-Hartman:
o I2C: remove devinitdata marking from i2c-nforce2.c as it's wrong
o I2C: consolidate the i2c delay functions
o I2C: minor cleanups to the i2c-nforce2 driver

Quoted 4 lines

Quoted 2 lines
Jan Dittmer:
o I2C: convert via686a temp_* to milli degree celsius
Does any of these apply to our CVS repository?

Probably :)

The memory leak from Daniele is probably needed, and the i2c delay cleanup can also be moved there. I don't think the others really apply though. I've put the individual patches in my kernel.org account if people want to see them (sorry for not posting them to the list, but I already have, this was just a re-send of these patches to Linus.)

thanks,

greg k-h

Mark M. Hoffman

2005-05-19 06:24 UTC #6 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

In reply to Greg KH

Quoted 24 lines · Greg KH

Quoted 23 lines · Jean Delvare
Greg,
Quoted 19 lines
<nikkne:hotpop.com>:
o I2C: fix Kconfig info
<patrick:dreker.de>:
o I2C: add ncforce2 i2c bus driver
Benjamin Herrenschmidt:
o I2C: timer clean up for i2c-keywest.c
Daniele Bellucci:
o I2C: fixed a little memory leak in i2c-ali15x3.c
Greg Kroah-Hartman:
o I2C: remove devinitdata marking from i2c-nforce2.c as it's wrong
o I2C: consolidate the i2c delay functions
o I2C: minor cleanups to the i2c-nforce2 driver
Jan Dittmer:
o I2C: convert via686a temp_* to milli degree celsius

Quoted 6 lines

Quoted 1 line
Does any of these apply to our CVS repository?
Probably :)
The memory leak from Daniele is probably needed, and the i2c delay cleanup can also be moved there. I don't think the others really apply

Done, plus most of the nforce2 patch. This bit though... any particular reason you didn't just whack all of it?

@@ -345,18 +334,12 @@
smbus->base, smbus->base+smbus->size-1, name);
return -1;
}
-
- /* TODO: find a better way to find out whether this file is compiled
- * with i2c 2.7.0 of earlier
- */
-/*#ifdef I2C_HW_SMBUS_AMD8111
+/*
smbus->adapter.owner = THIS_MODULE;
-#endif
-
smbus->adapter.id = I2C_ALGO_SMBUS | I2C_HW_SMBUS_NFORCE2; smbus->adapter.algo = &smbus_algorithm;
- smbus->adapter.algo_data = smbus;*/
-
+ smbus->adapter.algo_data = smbus;
+*/

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Mark M. Hoffman

2005-05-19 06:24 UTC #1 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

In reply to Greg KH

Quoted 20 lines · Greg KH

Quoted 19 lines · Mark M. Hoffman

Done, plus most of the nforce2 patch. This bit though... any particular reason you didn't just whack all of it?

(quoted diff) +1 -7

@@ -345,18 +334,12 @@
 			smbus->base, smbus->base+smbus->size-1, name);
 		return -1;
 	}
-
-	/* TODO: find a better way to find out whether this file is compiled
-	 * with i2c 2.7.0 of earlier
-	 */
-/*#ifdef I2C_HW_SMBUS_AMD8111
+/*
 	smbus->adapter.owner = THIS_MODULE;
-#endif
-
 	smbus->adapter.id = I2C_ALGO_SMBUS | I2C_HW_SMBUS_NFORCE2;
 	smbus->adapter.algo = &smbus_algorithm;

Quoted 8 lines

Review line 342
Quoted 4 lines
(quoted diff) +2 -2
-	smbus->adapter.algo_data = smbus;*/
-
+	smbus->adapter.algo_data = smbus;
+*/
I didn't have the time to verify if this really should be set or not, so I left the main portion alone. Does this mean I can delete all of the commented out area for 2.6?

With a closer look, I think this driver was broken before and after your patch. The algo_data field has to be set (to smbus), because it's used in nforce2_access(). Notice the macros...

#define NVIDIA_SMB_PRTCL        (smbus->base + 0x00)    /* protocol, PEC */
#define NVIDIA_SMB_STS          (smbus->base + 0x01)    /* status */
#define NVIDIA_SMB_ADDR         (smbus->base + 0x02)    /* address */

I'm not a fan of that (I would give the macros a parameter) but whatever.

A new patch is attached; please consider...

[comment]
This patch restores a line that was wrongly removed. There are also some trivial cleanups. It applies & compiles vs. 2.6.0-test2-bk4. It's untested (no hardware here).
[/comment]

Regards,

Signature · 65 lines

Mark M. Hoffman
[email protected]

-------------- next part --------------
--- linux-2.6.0-test2/drivers/i2c/busses/i2c-nforce2.c.old	2003-08-04 23:01:39.000000000 -0400
+++ linux-2.6.0-test2/drivers/i2c/busses/i2c-nforce2.c	2003-08-04 23:07:33.000000000 -0400
@@ -51,10 +51,6 @@
 #ifndef PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS
 #define PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS   0x0064
 #endif
-/* TODO: sync with lm-sensors */
-#ifndef I2C_HW_SMBUS_NFORCE2
-#define I2C_HW_SMBUS_NFORCE2	0x0c
-#endif
 
 
 struct nforce2_smbus {
@@ -130,20 +126,10 @@
 	},
 };
 
-
-#if 0
-/* Internally used pause function */
-static void nforce2_do_pause(unsigned int amount)
-{
-	current->state = TASK_INTERRUPTIBLE;
-	schedule_timeout(amount);
-}
-#endif
-
 /* Return -1 on error. See smbus.h for more information */
-static s32 nforce2_access(struct i2c_adapter * adap, u16 addr, unsigned short flags,
-		char read_write, u8 command, int size,
-		union i2c_smbus_data * data)
+static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
+		unsigned short flags, char read_write,
+		u8 command, int size, union i2c_smbus_data * data)
 {
 	struct nforce2_smbus *smbus = adap->algo_data;
 	unsigned char protocol, pec, temp;
@@ -251,7 +237,7 @@
 
 #if 0
 	do {
-		nforce2_do_pause(1);
+		i2c_do_pause(1);
 		temp = inb_p(NVIDIA_SMB_STS);
 	} while (((temp & NVIDIA_SMB_STS_DONE) = 0) && (timeout++ < MAX_TIMEOUT));
 #endif
@@ -334,13 +320,8 @@
 			smbus->base, smbus->base+smbus->size-1, name);
 		return -1;
 	}
-/*
-	smbus->adapter.owner = THIS_MODULE;
-	smbus->adapter.id = I2C_ALGO_SMBUS | I2C_HW_SMBUS_NFORCE2;
-	smbus->adapter.algo = &smbus_algorithm;
-	smbus->adapter.algo_data = smbus;
-*/
 	smbus->adapter = nforce2_adapter;
+	smbus->adapter.algo_data = smbus;
 	smbus->adapter.dev.parent = &dev->dev;
 	snprintf(smbus->adapter.dev.name, DEVICE_NAME_SIZE,
 		"SMBus nForce2 adapter at %04x", smbus->base);

Greg KH

author

2005-05-19 06:24 UTC #7 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test2

In reply to Greg KH

Quoted 13 lines · Mark M. Hoffman

Done, plus most of the nforce2 patch. This bit though... any particular reason you didn't just whack all of it?
(quoted diff) +0 -5
@@ -345,18 +334,12 @@
 			smbus->base, smbus->base+smbus->size-1, name);
 		return -1;
 	}
-
-	/* TODO: find a better way to find out whether this file is compiled
-	 * with i2c 2.7.0 of earlier
-	 */
-/*#ifdef I2C_HW_SMBUS_AMD8111

Review line 342

Quoted 10 lines

(quoted diff) +3 -4

+/*
 	smbus->adapter.owner = THIS_MODULE;
-#endif
-
 	smbus->adapter.id = I2C_ALGO_SMBUS | I2C_HW_SMBUS_NFORCE2;
 	smbus->adapter.algo = &smbus_algorithm;
-	smbus->adapter.algo_data = smbus;*/
-
+	smbus->adapter.algo_data = smbus;
+*/

I didn't have the time to verify if this really should be set or not, so I left the main portion alone. Does this mean I can delete all of the commented out area for 2.6?

thanks,

greg k-h

Mark M. Hoffman

2005-05-19 06:24 UTC #8 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test5

In reply to Greg KH

Quoted 8 lines · Greg KH

Here are some i2c driver fixes and additions for 2.6.0-test5. I've ported almost all of the i2c bus drivers from the 2.4 cvs tree to the 2.6 kernel (the one exception is a crazy one that acts like a pci hotplug driver in its init function, I'll leave that to someone else...) I've also cleaned up the existing i2c bus drivers by moving them into the proper directory, and polishing up the Kconfig entries.
There are a also a few other minor i2c cleanups and fixes in here.

Hi Greg:

To help me digest your patch, I generated some diffs from sensors CVS to linux-2.6.0-test5 + your patch. You can find them (and the script I used to generate them) here:

http://members.dca.net/mhoffman/sensors/diff_cvs-to-lk2.6/

The following quoted diffs are from the above link...

(drivers/i2c/busses/i2c-i810.c)

Review

Quoted 2 lines

(quoted diff) +1 -1

-static unsigned long ioaddr;
+static void *ioaddr;

But later we're doing arithmetic on ioaddr(!) Apparently the readl/writel macros are recasting that... but to me this is an obfuscating change.

Quoted 25 lines

(quoted diff) +1 -9

@@ -261,49 +225,32 @@
 {
        i2c_bit_del_bus(&i810_ddc_adapter);
        i2c_bit_del_bus(&i810_i2c_adapter);
+       iounmap(ioaddr);
 }
 
-
-/* Don't register driver to avoid driver conflicts */
-/*
 static struct pci_driver i810_driver = {
        .name           = "i810 smbus",
        .id_table       = i810_ids,
        .probe          = i810_probe,
        .remove         = __devexit_p(i810_remove),
 };
-*/
 
 static int __init i2c_i810_init(void)
 {
-       struct pci_dev *dev;
-       const struct pci_device_id *id;
-
-       printk("i2c-i810.o version %s (%s)\n", LM_VERSION, LM_DATE);
-/*

Review line 241

Quoted 10 lines

(quoted diff) +0 -8

        return pci_module_init(&i810_driver);
-*/
-       pci_for_each_dev(dev) {
-               id = pci_match_device(i810_ids, dev);
-               if(id)
-                       if(i810_probe(dev, id) >= 0)
-                               return 0;
-       }
-       return -ENODEV;
 }

Didn't you advise mds earlier that the correct solution here is to combine this code into the driver with which it now conflicts? (See drivers/video/i810/...) Likewise for i2c-via.c and i2c-voodoo3.c.

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Greg KH

author

2005-05-19 06:24 UTC #9 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test5

In reply to Greg KH

Quoted 7 lines · Mark M. Hoffman

(drivers/i2c/busses/i2c-i810.c)
Review
Quoted 2 lines
(quoted diff) +1 -1
-static unsigned long ioaddr;
+static void *ioaddr;
But later we're doing arithmetic on ioaddr(!) Apparently the readl/writel macros are recasting that... but to me this is an obfuscating change.

I did it this way, as ioremap returns a void *. And it's ok to do math on a void pointer, works the same way as math on a char *. It got rid of two casts in the driver with this change.

Quoted 31 lines

(quoted diff) +1 ~~-14~~

@@ -261,49 +225,32 @@
 {
        i2c_bit_del_bus(&i810_ddc_adapter);
        i2c_bit_del_bus(&i810_i2c_adapter);
+       iounmap(ioaddr);
 }
 
-
-/* Don't register driver to avoid driver conflicts */
-/*
 static struct pci_driver i810_driver = {
        .name           = "i810 smbus",
        .id_table       = i810_ids,
        .probe          = i810_probe,
        .remove         = __devexit_p(i810_remove),
 };
-*/
 
 static int __init i2c_i810_init(void)
 {
-       struct pci_dev *dev;
-       const struct pci_device_id *id;
-
-       printk("i2c-i810.o version %s (%s)\n", LM_VERSION, LM_DATE);
-/*
        return pci_module_init(&i810_driver);
-*/
-       pci_for_each_dev(dev) {
-               id = pci_match_device(i810_ids, dev);
-               if(id)
-                       if(i810_probe(dev, id) >= 0)

Quoted 8 lines

Review line 241
Quoted 4 lines
(quoted diff) +0 -3
-                               return 0;
-       }
-       return -ENODEV;
 }
Didn't you advise mds earlier that the correct solution here is to combine this code into the driver with which it now conflicts? (See drivers/video/i810/...) Likewise for i2c-via.c and i2c-voodoo3.c.

I did say this. But just "grabbing" the device like you are doing in 2.4 isn't an ok thing to do (the device can disappear underneath you quite easily in pci hotplug systems.) It is not safe to do this at all.

So if people complain about both drivers trying to grab the device, then people will work on merging the drivers properly, and will send me a patch :)

There are also some devices that these drivers match up to, that do not match up to some existing video drivers, from what I could tell, so it isn't a complete overlap.

Thanks for looking over the changes.

greg k-h

Jean Delvare

2005-05-19 06:24 UTC #10 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test5

In reply to Greg KH

Quoted 4 lines

I did it this way, as ioremap returns a void *. And it's ok to do math on a void pointer, works the same way as math on a char *. It got rid of two casts in the driver with this change.

Not too sure abour that. Gcc wouldn't have a flag (-Wcast-arith) for this without a reason.

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Mark M. Hoffman

2005-05-19 06:24 UTC #11 lore raw

PATCH 1/1 i2c driver fixes for 2.6.0-test5

In reply to Greg KH

Quoted 3 lines · Greg KH

Quoted 3 lines
I did it this way, as ioremap returns a void *. And it's ok to do math on a void pointer, works the same way as math on a char *. It got rid of two casts in the driver with this change.

Quoted 2 lines · Jean Delvare

Not too sure abour that. Gcc wouldn't have a flag (-Wcast-arith) for this without a reason.

Sorry, at my day job I work with compilers that reject that. Apparently gcc will reject it too if you specify -pedantic. Funny how this is ok in linux but // comments (legal C99) are vigorously removed. ;)

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]