* Jean Delvare <khali@xxxxxxxxxxxx> [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@xxxxxxxxxxx>
> > 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@xxxxxxxxx>
> > Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
