[patch 9/9]Four new i2c drivers and __init/__exit cleanup to i2c

[patch 9/9]Four new i2c drivers and __init/__exit cleanup to i2c

[Albert Cranford]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 484 bytes --]

Hello Linus,
New I2C drivers that have been adjusted after Russell King comments of August.
o i2c-algo-8xx.c
o i2c-pport.c
o i2c-adap-ibm_ocp.c
o i2c-pcf-epp.c
o Add new drivers to Config.in and Makefile.
o Add new drivers to i2c-core for initialization.
o Remove EXPORT_NO_SYMBOLS statement from i2c-dev, i2c-elektor and i2c-frodo.
o Cleanup init_module and cleanup_module adding __init and __exit to most drivers.
o Adjust i2c-elektor with cli/sti replacement.
-- 
ac9410@attbi.com

[-- Attachment #2: Type: TEXT/PLAIN, Size: 449 bytes --]

--- linux/drivers/i2c/i2c-elektor.c.orig	2002-09-14 22:10:45.000000000 -0400
+++ linux-2.5.34/drivers/i2c/i2c-elektor.c	2002-09-15 01:18:55.000000000 -0400
@@ -125,12 +125,12 @@
 	int timeout = 2;
 
 	if (irq > 0) {
-		cli();
+		local_irq_disable();
 		if (pcf_pending == 0) {
 			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
 		} else
 			pcf_pending = 0;
-		sti();
+		local_irq_enable();
 	} else {
 		udelay(100);
 	}

Re: [patch 9/9]Four new i2c drivers and __init/__exit cleanup to i2c

[Jeff Garzik]

Albert Cranford wrote:
> --- linux/drivers/i2c/i2c-elektor.c.orig	2002-09-14 22:10:45.000000000 -0400
> +++ linux-2.5.34/drivers/i2c/i2c-elektor.c	2002-09-15 01:18:55.000000000 -0400
> @@ -125,12 +125,12 @@
>  	int timeout = 2;
>  
>  	if (irq > 0) {
> -		cli();
> +		local_irq_disable();
>  		if (pcf_pending == 0) {
>  			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
>  		} else
>  			pcf_pending = 0;
> -		sti();
> +		local_irq_enable();
>  	} else {
>  		udelay(100);
>  	}



this is _not_ the way to fix...   use a proper spinlock

Re: [patch 9/9]Four new i2c drivers and __init/__exit cleanup to i2c

[Russell King]

On Sun, Sep 15, 2002 at 07:18:46PM -0400, Jeff Garzik wrote:
> Albert Cranford wrote:
> > --- linux/drivers/i2c/i2c-elektor.c.orig	2002-09-14 22:10:45.000000000 -0400
> > +++ linux-2.5.34/drivers/i2c/i2c-elektor.c	2002-09-15 01:18:55.000000000 -0400
> > @@ -125,12 +125,12 @@
> >  	int timeout = 2;
> >  
> >  	if (irq > 0) {
> > -		cli();
> > +		local_irq_disable();
> >  		if (pcf_pending == 0) {
> >  			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
> >  		} else
> >  			pcf_pending = 0;
> > -		sti();
> > +		local_irq_enable();
> >  	} else {
> >  		udelay(100);
> >  	}
> 
> 
> 
> this is _not_ the way to fix...   use a proper spinlock

You can't hold a spinlock and sleep though, was one of my points back
in August.  (Albert submitted a patch with all cli()/sti() converted
to spin_lock_irqsave()/spin_unlock_irqrestore().)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch 9/9]Four new i2c drivers and __init/__exit cleanup to i2c

[Jeff Garzik]

Russell King wrote:
> On Sun, Sep 15, 2002 at 07:18:46PM -0400, Jeff Garzik wrote:
> 
>>Albert Cranford wrote:
>>
>>>--- linux/drivers/i2c/i2c-elektor.c.orig	2002-09-14 22:10:45.000000000 -0400
>>>+++ linux-2.5.34/drivers/i2c/i2c-elektor.c	2002-09-15 01:18:55.000000000 -0400
>>>@@ -125,12 +125,12 @@
>>> 	int timeout = 2;
>>> 
>>> 	if (irq > 0) {
>>>-		cli();
>>>+		local_irq_disable();
>>> 		if (pcf_pending == 0) {
>>> 			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
>>> 		} else
>>> 			pcf_pending = 0;
>>>-		sti();
>>>+		local_irq_enable();
>>> 	} else {
>>> 		udelay(100);
>>> 	}
>>
>>
>>
>>this is _not_ the way to fix...   use a proper spinlock
> 
> 
> You can't hold a spinlock and sleep though, was one of my points back
> in August.  (Albert submitted a patch with all cli()/sti() converted
> to spin_lock_irqsave()/spin_unlock_irqrestore().)
> 


whoops, you're right.

That follows along with my suggestion in another email, then :)  use a 
semaphore.  The timeout can be handled with a kernel timer.  The timeout 
is clearly multiple seconds, so there's no fine grain involved.

AND, since the timeout is multiple seconds, the code should not be 
disable interrupts for that long anyway.

	Jeff