I2C: Fix unhandled fault in i2c-omap controller

I2C: Fix unhandled fault in i2c-omap controller

[Tony Lindgren]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Hi Jean,

Here's an omap I2C fix that would be nice to get into 2.6.25 if still
possible.

Regards,

Tony

[-- Attachment #2: i2c-clock-race --]
[-- Type: text/plain, Size: 4083 bytes --]

commit 8e4f19286e68d38d589451ad2a10545f2c40032d
Author: Tony Lindgren <tony@atomide.com>
Date:   Mon Feb 18 20:16:00 2008 -0800

    I2C: Fix unhandled fault in i2c-omap controller
    
    If an I2C interrupt happens between disabling interface clock
    and functional clock, the interrupt handler will produce an
    external abort on non-linefetch error when trying to access
    driver registers while interface clock is disabled.
    
    This patch fixes the problem by saving and disabling i2c-omap
    interrupt before turning off the clocks. Also disable functional
    clock before the interface clock as suggested by Paul Walmsley.
    
    Patch also renames enable/disable_clocks functions to unidle/idle
    functions. Note that the driver is currently not taking advantage
    of the idle interrupts. To use the idle interrupts, driver would
    have to enable interface clock based on the idle interrupt
    and dev->idle flag.
    
    Cc: Paul Walmsley <paul@pwsan.com>
    Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index da66397..862dd69 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -128,6 +128,8 @@ struct omap_i2c_dev {
 	size_t			buf_len;
 	struct i2c_adapter	adapter;
 	unsigned		rev1:1;
+	unsigned		idle:1;
+	u16			iestate;	/* Saved interrupt register */
 };
 
 static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
@@ -174,18 +176,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
 	}
 }
 
-static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
+static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
 	if (dev->iclk != NULL)
 		clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
+	if (dev->iestate)
+		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+	dev->idle = 0;
 }
 
-static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
+static void omap_i2c_idle(struct omap_i2c_dev *dev)
 {
+	u16 iv;
+
+	dev->idle = 1;
+	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
+	if (dev->rev1)
+		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
+	else
+		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
+	clk_disable(dev->fclk);
 	if (dev->iclk != NULL)
 		clk_disable(dev->iclk);
-	clk_disable(dev->fclk);
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	omap_i2c_enable_clocks(dev);
+	omap_i2c_unidle(dev);
 
 	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
 		goto out;
@@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (r == 0)
 		r = num;
 out:
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 	return r;
 }
 
@@ -403,6 +417,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 iv, w;
 
+	if (dev->idle)
+		return IRQ_NONE;
+
 	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
 	switch (iv) {
 	case 0x00:	/* None */
@@ -457,6 +474,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	u16 stat, w;
 	int count = 0;
 
+	if (dev->idle)
+		return IRQ_NONE;
+
 	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
 		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
@@ -575,7 +595,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	if ((r = omap_i2c_get_clocks(dev)) != 0)
 		goto err_free_mem;
 
-	omap_i2c_enable_clocks(dev);
+	omap_i2c_unidle(dev);
 
 	if (cpu_is_omap15xx())
 		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
@@ -610,7 +630,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 
 	return 0;
 
@@ -618,7 +638,7 @@ err_free_irq:
 	free_irq(dev->irq, dev);
 err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 	omap_i2c_put_clocks(dev);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);

Re: I2C: Fix unhandled fault in i2c-omap controller

[Jean Delvare]

Hi Tony,

On Thu, 13 Mar 2008 17:51:24 +0200, Tony Lindgren wrote:
> Here's an omap I2C fix that would be nice to get into 2.6.25 if still
> possible.

Yeah, that's possible, however the patch isn't totally trivial so I
would appreciate if someone from the OMAP community could review it
publicly and ack it. Then I'll push it to Linus quickly.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: I2C: Fix unhandled fault in i2c-omap controller

[Tony Lindgren]

* Jean Delvare <khali@linux-fr.org> [080314 17:53]:
> Hi Tony,
> 
> On Thu, 13 Mar 2008 17:51:24 +0200, Tony Lindgren wrote:
> > Here's an omap I2C fix that would be nice to get into 2.6.25 if still
> > possible.
> 
> Yeah, that's possible, however the patch isn't totally trivial so I
> would appreciate if someone from the OMAP community could review it
> publicly and ack it. Then I'll push it to Linus quickly.

Well we've had this fix tested in linux-omap for about a month now and
it really fixes the problem. I'll add that to the comments.

I don't know if anybody else has seen this problem repeatedly on omaps
as it's quite rare condition, but if somebody else was seeing it,
please Ack here! :)

Regards,

Tony

Re: I2C: Fix unhandled fault in i2c-omap controller

[Jean Delvare]

On Thu, 13 Mar 2008 17:51:24 +0200, Tony Lindgren wrote:
> Hi Jean,
> 
> Here's an omap I2C fix that would be nice to get into 2.6.25 if still
> possible.

Review:

> commit 8e4f19286e68d38d589451ad2a10545f2c40032d
> Author: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date:   Mon Feb 18 20:16:00 2008 -0800
> 
>     I2C: Fix unhandled fault in i2c-omap controller
>     
>     If an I2C interrupt happens between disabling interface clock
>     and functional clock, the interrupt handler will produce an
>     external abort on non-linefetch error when trying to access
>     driver registers while interface clock is disabled.
>     
>     This patch fixes the problem by saving and disabling i2c-omap
>     interrupt before turning off the clocks. Also disable functional
>     clock before the interface clock as suggested by Paul Walmsley.
>     
>     Patch also renames enable/disable_clocks functions to unidle/idle
>     functions. Note that the driver is currently not taking advantage
>     of the idle interrupts. To use the idle interrupts, driver would
>     have to enable interface clock based on the idle interrupt
>     and dev->idle flag.
>     
>     Cc: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index da66397..862dd69 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -128,6 +128,8 @@ struct omap_i2c_dev {
>  	size_t			buf_len;
>  	struct i2c_adapter	adapter;
>  	unsigned		rev1:1;
> +	unsigned		idle:1;
> +	u16			iestate;	/* Saved interrupt register */
>  };
>  
>  static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> @@ -174,18 +176,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
>  	}
>  }
>  
> -static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
> +static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  {
>  	if (dev->iclk != NULL)
>  		clk_enable(dev->iclk);
>  	clk_enable(dev->fclk);
> +	if (dev->iestate)
> +		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> +	dev->idle = 0;
>  }
>  
> -static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
> +static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  {
> +	u16 iv;
> +
> +	dev->idle = 1;
> +	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> +	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
> +	if (dev->rev1)
> +		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);

Reading OMAP_I2C_IV_REG but not doing anything with the value? This
deserves at least a comment if this is done on purpose (but then I
don't think you need the iv variable at all.)

> +	else
> +		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
> +	clk_disable(dev->fclk);
>  	if (dev->iclk != NULL)
>  		clk_disable(dev->iclk);
> -	clk_disable(dev->fclk);
>  }
>  
>  static int omap_i2c_init(struct omap_i2c_dev *dev)
> @@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int i;
>  	int r;
>  
> -	omap_i2c_enable_clocks(dev);
> +	omap_i2c_unidle(dev);
>  
>  	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
>  		goto out;
> @@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	if (r == 0)
>  		r = num;
>  out:
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  	return r;
>  }
>  
> @@ -403,6 +417,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>  	struct omap_i2c_dev *dev = dev_id;
>  	u16 iv, w;
>  
> +	if (dev->idle)
> +		return IRQ_NONE;
> +
>  	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
>  	switch (iv) {
>  	case 0x00:	/* None */
> @@ -457,6 +474,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  	u16 stat, w;
>  	int count = 0;
>  
> +	if (dev->idle)
> +		return IRQ_NONE;
> +
>  	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
>  	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
>  		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> @@ -575,7 +595,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  	if ((r = omap_i2c_get_clocks(dev)) != 0)
>  		goto err_free_mem;
>  
> -	omap_i2c_enable_clocks(dev);
> +	omap_i2c_unidle(dev);
>  
>  	if (cpu_is_omap15xx())
>  		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
> @@ -610,7 +630,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
>  
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  
>  	return 0;
>  
> @@ -618,7 +638,7 @@ err_free_irq:
>  	free_irq(dev->irq, dev);
>  err_unuse_clocks:
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  	omap_i2c_put_clocks(dev);
>  err_free_mem:
>  	platform_set_drvdata(pdev, NULL);

The rest of the patch looks OK to me.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: I2C: Fix unhandled fault in i2c-omap controller

[Tony Lindgren]

* Jean Delvare <khali@linux-fr.org> [080314 19:52]:
> On Thu, 13 Mar 2008 17:51:24 +0200, Tony Lindgren wrote:
> > Hi Jean,
> > 
> > Here's an omap I2C fix that would be nice to get into 2.6.25 if still
> > possible.
> 
> Review:

Thanks for looking throught it.

> 
> > commit 8e4f19286e68d38d589451ad2a10545f2c40032d
> > Author: Tony Lindgren <tony@atomide.com>
> > Date:   Mon Feb 18 20:16:00 2008 -0800
> > 
> >     I2C: Fix unhandled fault in i2c-omap controller
> >     
> >     If an I2C interrupt happens between disabling interface clock
> >     and functional clock, the interrupt handler will produce an
> >     external abort on non-linefetch error when trying to access
> >     driver registers while interface clock is disabled.
> >     
> >     This patch fixes the problem by saving and disabling i2c-omap
> >     interrupt before turning off the clocks. Also disable functional
> >     clock before the interface clock as suggested by Paul Walmsley.
> >     
> >     Patch also renames enable/disable_clocks functions to unidle/idle
> >     functions. Note that the driver is currently not taking advantage
> >     of the idle interrupts. To use the idle interrupts, driver would
> >     have to enable interface clock based on the idle interrupt
> >     and dev->idle flag.
> >     
> >     Cc: Paul Walmsley <paul@pwsan.com>
> >     Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index da66397..862dd69 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -128,6 +128,8 @@ struct omap_i2c_dev {
> >  	size_t			buf_len;
> >  	struct i2c_adapter	adapter;
> >  	unsigned		rev1:1;
> > +	unsigned		idle:1;
> > +	u16			iestate;	/* Saved interrupt register */
> >  };
> >  
> >  static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> > @@ -174,18 +176,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
> >  	}
> >  }
> >  
> > -static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
> > +static void omap_i2c_unidle(struct omap_i2c_dev *dev)
> >  {
> >  	if (dev->iclk != NULL)
> >  		clk_enable(dev->iclk);
> >  	clk_enable(dev->fclk);
> > +	if (dev->iestate)
> > +		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> > +	dev->idle = 0;
> >  }
> >  
> > -static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
> > +static void omap_i2c_idle(struct omap_i2c_dev *dev)
> >  {
> > +	u16 iv;
> > +
> > +	dev->idle = 1;
> > +	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> > +	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
> > +	if (dev->rev1)
> > +		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
> 
> Reading OMAP_I2C_IV_REG but not doing anything with the value? This
> deserves at least a comment if this is done on purpose (but then I
> don't think you need the iv variable at all.)

Yeah for rev1 of the controller reading of IV (Interrupt Vector)
clears the interrupts. I'll add a comment there.

> 
> > +	else
> > +		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
> > +	clk_disable(dev->fclk);
> >  	if (dev->iclk != NULL)
> >  		clk_disable(dev->iclk);
> > -	clk_disable(dev->fclk);
> >  }
> >  
> >  static int omap_i2c_init(struct omap_i2c_dev *dev)
> > @@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	int i;
> >  	int r;
> >  
> > -	omap_i2c_enable_clocks(dev);
> > +	omap_i2c_unidle(dev);
> >  
> >  	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
> >  		goto out;
> > @@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	if (r == 0)
> >  		r = num;
> >  out:
> > -	omap_i2c_disable_clocks(dev);
> > +	omap_i2c_idle(dev);
> >  	return r;
> >  }
> >  
> > @@ -403,6 +417,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> >  	struct omap_i2c_dev *dev = dev_id;
> >  	u16 iv, w;
> >  
> > +	if (dev->idle)
> > +		return IRQ_NONE;
> > +
> >  	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
> >  	switch (iv) {
> >  	case 0x00:	/* None */
> > @@ -457,6 +474,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >  	u16 stat, w;
> >  	int count = 0;
> >  
> > +	if (dev->idle)
> > +		return IRQ_NONE;
> > +
> >  	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> >  	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> >  		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> > @@ -575,7 +595,7 @@ omap_i2c_probe(struct platform_device *pdev)
> >  	if ((r = omap_i2c_get_clocks(dev)) != 0)
> >  		goto err_free_mem;
> >  
> > -	omap_i2c_enable_clocks(dev);
> > +	omap_i2c_unidle(dev);
> >  
> >  	if (cpu_is_omap15xx())
> >  		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
> > @@ -610,7 +630,7 @@ omap_i2c_probe(struct platform_device *pdev)
> >  		goto err_free_irq;
> >  	}
> >  
> > -	omap_i2c_disable_clocks(dev);
> > +	omap_i2c_idle(dev);
> >  
> >  	return 0;
> >  
> > @@ -618,7 +638,7 @@ err_free_irq:
> >  	free_irq(dev->irq, dev);
> >  err_unuse_clocks:
> >  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> > -	omap_i2c_disable_clocks(dev);
> > +	omap_i2c_idle(dev);
> >  	omap_i2c_put_clocks(dev);
> >  err_free_mem:
> >  	platform_set_drvdata(pdev, NULL);
> 
> The rest of the patch looks OK to me.
> 
> -- 
> Jean Delvare

[PATCH] I2C: Fix unhandled fault in i2c-omap controller, take #2

[Tony Lindgren]

[-- Attachment #1: Type: text/plain, Size: 48 bytes --]

Hi,

Here's an updated version

Regards,

Tony


[-- Attachment #2: i2c-clock-race-v2 --]
[-- Type: text/plain, Size: 4211 bytes --]

commit 94a3ce6a20c3920e0e43991d38506134791e9d72
Author: Tony Lindgren <tony@atomide.com>
Date:   Mon Feb 18 20:16:00 2008 -0800

    I2C: Fix unhandled fault in i2c-omap controller
    
    If an I2C interrupt happens between disabling interface clock
    and functional clock, the interrupt handler will produce an
    external abort on non-linefetch error when trying to access
    driver registers while interface clock is disabled.
    
    This patch fixes the problem by saving and disabling i2c-omap
    interrupt before turning off the clocks. Also disable functional
    clock before the interface clock as suggested by Paul Walmsley.
    
    Patch also renames enable/disable_clocks functions to unidle/idle
    functions. Note that the driver is currently not taking advantage
    of the idle interrupts. To use the idle interrupts, driver would
    have to enable interface clock based on the idle interrupt
    and dev->idle flag.
    
    This patch has been tested in linux-omap tree with various omaps.
    
    Cc: linux-omap@vger.kernel.org
    Cc: Paul Walmsley <paul@pwsan.com>
    Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index da66397..7ba3177 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -128,6 +128,8 @@ struct omap_i2c_dev {
 	size_t			buf_len;
 	struct i2c_adapter	adapter;
 	unsigned		rev1:1;
+	unsigned		idle:1;
+	u16			iestate;	/* Saved interrupt register */
 };
 
 static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
@@ -174,18 +176,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
 	}
 }
 
-static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
+static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
 	if (dev->iclk != NULL)
 		clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
+	if (dev->iestate)
+		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+	dev->idle = 0;
 }
 
-static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
+static void omap_i2c_idle(struct omap_i2c_dev *dev)
 {
+	u16 iv;
+
+	dev->idle = 1;
+	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
+	if (dev->rev1)
+		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);	/* Read clears */
+	else
+		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
+	clk_disable(dev->fclk);
 	if (dev->iclk != NULL)
 		clk_disable(dev->iclk);
-	clk_disable(dev->fclk);
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	omap_i2c_enable_clocks(dev);
+	omap_i2c_unidle(dev);
 
 	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
 		goto out;
@@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (r == 0)
 		r = num;
 out:
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 	return r;
 }
 
@@ -403,6 +417,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 iv, w;
 
+	if (dev->idle)
+		return IRQ_NONE;
+
 	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
 	switch (iv) {
 	case 0x00:	/* None */
@@ -457,6 +474,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	u16 stat, w;
 	int count = 0;
 
+	if (dev->idle)
+		return IRQ_NONE;
+
 	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
 		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
@@ -575,7 +595,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	if ((r = omap_i2c_get_clocks(dev)) != 0)
 		goto err_free_mem;
 
-	omap_i2c_enable_clocks(dev);
+	omap_i2c_unidle(dev);
 
 	if (cpu_is_omap15xx())
 		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
@@ -610,7 +630,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 
 	return 0;
 
@@ -618,7 +638,7 @@ err_free_irq:
 	free_irq(dev->irq, dev);
 err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-	omap_i2c_disable_clocks(dev);
+	omap_i2c_idle(dev);
 	omap_i2c_put_clocks(dev);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);

Re: [PATCH] I2C: Fix unhandled fault in i2c-omap controller, take #2

[David Brownell]

On Monday 17 March 2008, Tony Lindgren wrote:
> @@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>         int i;
>         int r;
>  
> -       omap_i2c_enable_clocks(dev);
> +       omap_i2c_unidle(dev);
>  
>         if ((r = omap_i2c_wait_for_bb(dev)) < 0)
>                 goto out;
> @@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>         if (r == 0)
>                 r = num;
>  out:
> -       omap_i2c_disable_clocks(dev);
> +       omap_i2c_idle(dev);
>         return r;
>  }
>  

Wouldn't it be nice -- from a pure power management perspective
of course! -- if more drivers were this careful about idling
their clocks when they have no work to do.  ;)

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] I2C: Fix unhandled fault in i2c-omap controller, take #2

[Jean Delvare]

On Mon, 17 Mar 2008 09:19:49 +0200, Tony Lindgren wrote:
> Hi,
> 
> Here's an updated version

Applied and queued for 2.6.25, thanks.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c