Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 2, 2009 at 10:11 PM, Wolfram Sang wrote: > On Fri, Oct 02, 2009 at 04:17:09PM +0800, Richard Zhao wrote: >> On Fri, Oct 2, 2009 at 3:26 PM, Sascha Hauer wrote: >> > On Fri, Oct 02, 2009 at 08:57:04AM +0800, Richard Zhao wrote: >> >> On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang >> >> wrote: >> >> >> > Ah, so 'make the driver work on i.MX51' is a good statement which >> >> >> > should >> >> >> > be part of the commit message. >> >> >> Well, maybe I can mention it. >> >> >> But I think the good point is to present what you modified, not the >> >> >> side effect. >> >> > >> >> > It is not the side effect but the intention :) As no code is changed >> >> > without a >> >> > need, the reason really should be in the patch description. >> >> No, it's not intention. I'm just trying to make the controller work in >> >> a right way. Without this patch, maybe some other fast cpus have >> >> problem too. I just tested mx31 and mx51. I will add "Without this >> >> patch, i2c on some fast SoCs (for example imx51) will not work". Is it >> >> ok for you? >> > >> > Please remember that we do not have i.MX51 support in mainline, so this >> > is irrelevant atm. >> So I don't need to meantion mx51? "Without this patch, i2c on some >> fast SoCs will not work" is all right? > > Sounds good to me! > >> > At the moment we have a driver which is not multi master capable. >> > Looking at the datasheet the change you do seems not enough to change >> > this. So we should take a patch which changes something from which you >> > think it might be needed? And you don't even have the details at hand? >> > >> > No. >> Ok, It seems I have no reason to keep the busy wait before START. >> Wolfram, do you agree to remove the busy wait? > > If somebody really needs multi-master and is able to test it, then it can be > reimplemented (but as said, this needs a lot more changes). Have to have a > closer look though, if it may detect a stalled bus. > >> I saw you submmited the original driver. > > I helped in getting it accepted, but the commit is mainly from Darius. > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkrGCfoACgkQD27XaX1/VRtdtQCfQY/HExUvAyQbilCV+F83UZZ4 > /ZQAnicn78e46mlqE3Z/9xXFMW8B2QCA > =qYV9 > -END PGP SIGNATURE- > > Let me sumerize the comments: 1. Sascha, Wolfram: tell why add IBB check in commit message. Add something like "Without this patch, i2c on some fast SoCs will not work". Agree 2. Sascha: Remove IBB check before START. Agree. 3. Andrew: Optimize the busy wait loop. Agree. Thanks Richard -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 02, 2009 at 04:17:09PM +0800, Richard Zhao wrote: > On Fri, Oct 2, 2009 at 3:26 PM, Sascha Hauer wrote: > > On Fri, Oct 02, 2009 at 08:57:04AM +0800, Richard Zhao wrote: > >> On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang > >> wrote: > >> >> > Ah, so 'make the driver work on i.MX51' is a good statement which > >> >> > should > >> >> > be part of the commit message. > >> >> Well, maybe I can mention it. > >> >> But I think the good point is to present what you modified, not the > >> >> side effect. > >> > > >> > It is not the side effect but the intention :) As no code is changed > >> > without a > >> > need, the reason really should be in the patch description. > >> No, it's not intention. I'm just trying to make the controller work in > >> a right way. Without this patch, maybe some other fast cpus have > >> problem too. I just tested mx31 and mx51. I will add "Without this > >> patch, i2c on some fast SoCs (for example imx51) will not work". Is it > >> ok for you? > > > > Please remember that we do not have i.MX51 support in mainline, so this > > is irrelevant atm. > So I don't need to meantion mx51? "Without this patch, i2c on some > fast SoCs will not work" is all right? Sounds good to me! > > At the moment we have a driver which is not multi master capable. > > Looking at the datasheet the change you do seems not enough to change > > this. So we should take a patch which changes something from which you > > think it might be needed? And you don't even have the details at hand? > > > > No. > Ok, It seems I have no reason to keep the busy wait before START. > Wolfram, do you agree to remove the busy wait? If somebody really needs multi-master and is able to test it, then it can be reimplemented (but as said, this needs a lot more changes). Have to have a closer look though, if it may detect a stalled bus. > I saw you submmited the original driver. I helped in getting it accepted, but the commit is mainly from Darius. Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 2, 2009 at 3:26 PM, Sascha Hauer wrote: > On Fri, Oct 02, 2009 at 08:57:04AM +0800, Richard Zhao wrote: >> On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang wrote: >> >> > Ah, so 'make the driver work on i.MX51' is a good statement which should >> >> > be part of the commit message. >> >> Well, maybe I can mention it. >> >> But I think the good point is to present what you modified, not the side >> >> effect. >> > >> > It is not the side effect but the intention :) As no code is changed >> > without a >> > need, the reason really should be in the patch description. >> No, it's not intention. I'm just trying to make the controller work in >> a right way. Without this patch, maybe some other fast cpus have >> problem too. I just tested mx31 and mx51. I will add "Without this >> patch, i2c on some fast SoCs (for example imx51) will not work". Is it >> ok for you? > > Please remember that we do not have i.MX51 support in mainline, so this > is irrelevant atm. So I don't need to meantion mx51? "Without this patch, i2c on some fast SoCs will not work" is all right? > >> > >> >> Yes. But I don't have multi-master system. So I can't say that. >> >> The code is just taken from Freescale latest code. Without it, It >> >> could also cause a device error. I forget the details. Anyway, it >> >> doesn't make anything wrong. >> > >> > Do you know where the details are explained? >> No, I don't. I don't have device in hand now. If you have, could you >> please help do a simple test? >> Use hw to simulate multi-master system. Before execute xfer, you first >> pull down SDA, then pull down SDC. It simulates a START. and execute >> xfer to see whether IBB is set? > > No, we won't do any tests on hardware. > > At the moment we have a driver which is not multi master capable. > Looking at the datasheet the change you do seems not enough to change > this. So we should take a patch which changes something from which you > think it might be needed? And you don't even have the details at hand? > > No. Ok, It seems I have no reason to keep the busy wait before START. Wolfram, do you agree to remove the busy wait? I saw you submmited the original driver. Let get away from multi-master. There's nearly no multi-master i2c bus system in reality. Thanks Richard > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 02, 2009 at 08:57:04AM +0800, Richard Zhao wrote: > On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang wrote: > >> > Ah, so 'make the driver work on i.MX51' is a good statement which should > >> > be part of the commit message. > >> Well, maybe I can mention it. > >> But I think the good point is to present what you modified, not the side > >> effect. > > > > It is not the side effect but the intention :) As no code is changed > > without a > > need, the reason really should be in the patch description. > No, it's not intention. I'm just trying to make the controller work in > a right way. Without this patch, maybe some other fast cpus have > problem too. I just tested mx31 and mx51. I will add "Without this > patch, i2c on some fast SoCs (for example imx51) will not work". Is it > ok for you? Please remember that we do not have i.MX51 support in mainline, so this is irrelevant atm. > > > >> Yes. But I don't have multi-master system. So I can't say that. > >> The code is just taken from Freescale latest code. Without it, It > >> could also cause a device error. I forget the details. Anyway, it > >> doesn't make anything wrong. > > > > Do you know where the details are explained? > No, I don't. I don't have device in hand now. If you have, could you > please help do a simple test? > Use hw to simulate multi-master system. Before execute xfer, you first > pull down SDA, then pull down SDC. It simulates a START. and execute > xfer to see whether IBB is set? No, we won't do any tests on hardware. At the moment we have a driver which is not multi master capable. Looking at the datasheet the change you do seems not enough to change this. So we should take a patch which changes something from which you think it might be needed? And you don't even have the details at hand? No. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang wrote: >> > Ah, so 'make the driver work on i.MX51' is a good statement which should >> > be part of the commit message. >> Well, maybe I can mention it. >> But I think the good point is to present what you modified, not the side >> effect. > > It is not the side effect but the intention :) As no code is changed without a > need, the reason really should be in the patch description. No, it's not intention. I'm just trying to make the controller work in a right way. Without this patch, maybe some other fast cpus have problem too. I just tested mx31 and mx51. I will add "Without this patch, i2c on some fast SoCs (for example imx51) will not work". Is it ok for you? > >> Yes. But I don't have multi-master system. So I can't say that. >> The code is just taken from Freescale latest code. Without it, It >> could also cause a device error. I forget the details. Anyway, it >> doesn't make anything wrong. > > Do you know where the details are explained? No, I don't. I don't have device in hand now. If you have, could you please help do a simple test? Use hw to simulate multi-master system. Before execute xfer, you first pull down SDA, then pull down SDC. It simulates a START. and execute xfer to see whether IBB is set? Thanks Richard > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkrE2uEACgkQD27XaX1/VRumZQCeL4x9oaBKKjSKzJLlRrkfvvqg > nlEAoLpQdpI3TeKEvK13rs46kSZRDsZU > =7kM6 > -END PGP SIGNATURE- > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
> > Ah, so 'make the driver work on i.MX51' is a good statement which should > > be part of the commit message. > Well, maybe I can mention it. > But I think the good point is to present what you modified, not the side > effect. It is not the side effect but the intention :) As no code is changed without a need, the reason really should be in the patch description. > Yes. But I don't have multi-master system. So I can't say that. > The code is just taken from Freescale latest code. Without it, It > could also cause a device error. I forget the details. Anyway, it > doesn't make anything wrong. Do you know where the details are explained? Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 1, 2009 at 5:52 PM, Sascha Hauer wrote:
> On Thu, Oct 01, 2009 at 05:11:30PM +0800, Richard Zhao wrote:
>> On Thu, Oct 1, 2009 at 4:38 PM, Sascha Hauer wrote:
>> > On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote:
>> >> On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao wrote:
>> >> > After START/RESTART, wait for busy bit to be set and
>> >> > after STOP, wait for busy bit to be clear.
>> >> >
>> >> > Signed-off-by: Richard Zhao
>> >> >
>> >> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> >> > index 4afba3e..156cc95 100644
>> >> > --- a/drivers/i2c/busses/i2c-imx.c
>> >> > +++ b/drivers/i2c/busses/i2c-imx.c
>> >> > @@ -125,14 +125,19 @@ struct imx_i2c_struct {
>> >> > /** Functions for IMX I2C adapter driver
>> >> > ***
>> >> > ***/
>> >> >
>> >> > -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
>> >> > +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int
>> >> > for_busy)
>> >> > {
>> >> > unsigned long orig_jiffies = jiffies;
>> >> > + unsigned int temp;
>> >> >
>> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >> >
>> >> > - /* wait for bus not busy */
>> >> > - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
>> >> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
>> >> > + while (1) {
>> >> > + if (for_busy && (temp & I2SR_IBB))
>> >> > + break;
>> >> > + if (!for_busy && !(temp & I2SR_IBB))
>> >> > + break;
>> >> > if (signal_pending(current)) {
>> >> > dev_dbg(&i2c_imx->adapter.dev,
>> >> > "<%s> I2C Interrupted\n", __func__);
>> >> > @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
>> >> > *i2c_imx)
>> >> > return -EIO;
>> >> > }
>> >> > schedule();
>> >> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
>> >> > }
>> >> >
>> >> > return 0;
>> >> > @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct
>> >> > *i2c_imx)
>> >> > return 0;
>> >> > }
>> >> >
>> >> > -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>> >> > +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>> >> > {
>> >> > unsigned int temp = 0;
>> >> > + int result;
>> >> >
>> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >> >
>> >> > /* Enable I2C controller */
>> >> > + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
>> >> > writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
>> >> > +
>> >> > + result = i2c_imx_bus_busy(i2c_imx, 0);
>> >> > + if (result)
>> >> > + return result;
>> >> > +
>> >> > /* Start I2C transaction */
>> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> >> > temp |= I2CR_MSTA;
>> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> >> > + result = i2c_imx_bus_busy(i2c_imx, 1);
>> >> > + if (result)
>> >> > + return result;
>> >> > +
>> >> > temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> >> > + return result;
>> >> > }
>> >> >
>> >> > static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>> >> > @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct
>> >> > *i2c_imx)
>> >> > /* Stop I2C transaction */
>> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> >> > - temp &= ~I2CR_MSTA;
>> >> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
>> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> >> > - /* setup chip registers to defaults */
>> >> > - writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
>> >> > - writeb(0, i2c_imx->base + IMX_I2C_I2SR);
>> >> > /*
>> >> > * This delay caused by an i.MXL hardware bug.
>> >> > * If no (or too short) delay, no "STOP" bit will be generated.
>> >> > */
>> >> > udelay(i2c_imx->disable_delay);
>> >> > +
>> >> > + i2c_imx_bus_busy(i2c_imx, 0);
>> >> > +
>> >> > /* Disable I2C controller */
>> >> > writeb(0, i2c_imx->base + IMX_I2C_I2CR);
>> >> > }
>> >> > @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct
>> >> > *i2c_imx, struct i2c_msg *msgs)
>> >> > dev_dbg(&i2c_imx->adapter.dev,
>> >> > "<%s> clear MSTA\n", __func__);
>> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> >> > - temp &= ~I2CR_MSTA;
>> >> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
>> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> >> > }
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 01, 2009 at 05:11:30PM +0800, Richard Zhao wrote:
> On Thu, Oct 1, 2009 at 4:38 PM, Sascha Hauer wrote:
> > On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote:
> >> On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao wrote:
> >> > After START/RESTART, wait for busy bit to be set and
> >> > after STOP, wait for busy bit to be clear.
> >> >
> >> > Signed-off-by: Richard Zhao
> >> >
> >> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> >> > index 4afba3e..156cc95 100644
> >> > --- a/drivers/i2c/busses/i2c-imx.c
> >> > +++ b/drivers/i2c/busses/i2c-imx.c
> >> > @@ -125,14 +125,19 @@ struct imx_i2c_struct {
> >> > /** Functions for IMX I2C adapter driver
> >> > ***
> >> > ***/
> >> >
> >> > -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
> >> > +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int
> >> > for_busy)
> >> > {
> >> > unsigned long orig_jiffies = jiffies;
> >> > + unsigned int temp;
> >> >
> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >> >
> >> > - /* wait for bus not busy */
> >> > - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> >> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> >> > + while (1) {
> >> > + if (for_busy && (temp & I2SR_IBB))
> >> > + break;
> >> > + if (!for_busy && !(temp & I2SR_IBB))
> >> > + break;
> >> > if (signal_pending(current)) {
> >> > dev_dbg(&i2c_imx->adapter.dev,
> >> > "<%s> I2C Interrupted\n", __func__);
> >> > @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> >> > *i2c_imx)
> >> > return -EIO;
> >> > }
> >> > schedule();
> >> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> >> > }
> >> >
> >> > return 0;
> >> > @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct
> >> > *i2c_imx)
> >> > return 0;
> >> > }
> >> >
> >> > -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> >> > +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> >> > {
> >> > unsigned int temp = 0;
> >> > + int result;
> >> >
> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >> >
> >> > /* Enable I2C controller */
> >> > + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> >> > writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> >> > +
> >> > + result = i2c_imx_bus_busy(i2c_imx, 0);
> >> > + if (result)
> >> > + return result;
> >> > +
> >> > /* Start I2C transaction */
> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> >> > temp |= I2CR_MSTA;
> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> >> > + result = i2c_imx_bus_busy(i2c_imx, 1);
> >> > + if (result)
> >> > + return result;
> >> > +
> >> > temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> >> > + return result;
> >> > }
> >> >
> >> > static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> >> > @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct
> >> > *i2c_imx)
> >> > /* Stop I2C transaction */
> >> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> >> > - temp &= ~I2CR_MSTA;
> >> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> >> > - /* setup chip registers to defaults */
> >> > - writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> >> > - writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> >> > /*
> >> > * This delay caused by an i.MXL hardware bug.
> >> > * If no (or too short) delay, no "STOP" bit will be generated.
> >> > */
> >> > udelay(i2c_imx->disable_delay);
> >> > +
> >> > + i2c_imx_bus_busy(i2c_imx, 0);
> >> > +
> >> > /* Disable I2C controller */
> >> > writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> >> > }
> >> > @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct
> >> > *i2c_imx, struct i2c_msg *msgs)
> >> > dev_dbg(&i2c_imx->adapter.dev,
> >> > "<%s> clear MSTA\n", __func__);
> >> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> >> > - temp &= ~I2CR_MSTA;
> >> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
> >> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> >> > } else if (i == (msgs->len - 2)) {
> >> > dev_dbg(&i2c_imx->adapter.dev,
> >> > @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 1, 2009 at 4:38 PM, Sascha Hauer wrote:
> On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote:
>> On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao wrote:
>> > After START/RESTART, wait for busy bit to be set and
>> > after STOP, wait for busy bit to be clear.
>> >
>> > Signed-off-by: Richard Zhao
>> >
>> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> > index 4afba3e..156cc95 100644
>> > --- a/drivers/i2c/busses/i2c-imx.c
>> > +++ b/drivers/i2c/busses/i2c-imx.c
>> > @@ -125,14 +125,19 @@ struct imx_i2c_struct {
>> > /** Functions for IMX I2C adapter driver
>> > ***
>> > ***/
>> >
>> > -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
>> > +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>> > {
>> > unsigned long orig_jiffies = jiffies;
>> > + unsigned int temp;
>> >
>> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >
>> > - /* wait for bus not busy */
>> > - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
>> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
>> > + while (1) {
>> > + if (for_busy && (temp & I2SR_IBB))
>> > + break;
>> > + if (!for_busy && !(temp & I2SR_IBB))
>> > + break;
>> > if (signal_pending(current)) {
>> > dev_dbg(&i2c_imx->adapter.dev,
>> > "<%s> I2C Interrupted\n", __func__);
>> > @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
>> > *i2c_imx)
>> > return -EIO;
>> > }
>> > schedule();
>> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
>> > }
>> >
>> > return 0;
>> > @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct
>> > *i2c_imx)
>> > return 0;
>> > }
>> >
>> > -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>> > +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>> > {
>> > unsigned int temp = 0;
>> > + int result;
>> >
>> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >
>> > /* Enable I2C controller */
>> > + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
>> > writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
>> > +
>> > + result = i2c_imx_bus_busy(i2c_imx, 0);
>> > + if (result)
>> > + return result;
>> > +
>> > /* Start I2C transaction */
>> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> > temp |= I2CR_MSTA;
>> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> > + result = i2c_imx_bus_busy(i2c_imx, 1);
>> > + if (result)
>> > + return result;
>> > +
>> > temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> > + return result;
>> > }
>> >
>> > static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>> > @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct
>> > *i2c_imx)
>> > /* Stop I2C transaction */
>> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> > - temp &= ~I2CR_MSTA;
>> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
>> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> > - /* setup chip registers to defaults */
>> > - writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
>> > - writeb(0, i2c_imx->base + IMX_I2C_I2SR);
>> > /*
>> > * This delay caused by an i.MXL hardware bug.
>> > * If no (or too short) delay, no "STOP" bit will be generated.
>> > */
>> > udelay(i2c_imx->disable_delay);
>> > +
>> > + i2c_imx_bus_busy(i2c_imx, 0);
>> > +
>> > /* Disable I2C controller */
>> > writeb(0, i2c_imx->base + IMX_I2C_I2CR);
>> > }
>> > @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct
>> > *i2c_imx, struct i2c_msg *msgs)
>> > dev_dbg(&i2c_imx->adapter.dev,
>> > "<%s> clear MSTA\n", __func__);
>> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> > - temp &= ~I2CR_MSTA;
>> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
>> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> > } else if (i == (msgs->len - 2)) {
>> > dev_dbg(&i2c_imx->adapter.dev,
>> > @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>> >
>> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>> >
>> > - /* Check if i2c bus is not busy */
>> > - result = i2c_imx_bus_busy(i2c_imx);
>> > + /* Start I2C transfer */
>> > + result = i2c_imx_start(i2c_imx);
>> > if (result)
>> >
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote:
> On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao wrote:
> > After START/RESTART, wait for busy bit to be set and
> > after STOP, wait for busy bit to be clear.
> >
> > Signed-off-by: Richard Zhao
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 4afba3e..156cc95 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -125,14 +125,19 @@ struct imx_i2c_struct {
> > /** Functions for IMX I2C adapter driver
> > ***
> > ***/
> >
> > -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
> > +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> > {
> > unsigned long orig_jiffies = jiffies;
> > + unsigned int temp;
> >
> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >
> > - /* wait for bus not busy */
> > - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> > + while (1) {
> > + if (for_busy && (temp & I2SR_IBB))
> > + break;
> > + if (!for_busy && !(temp & I2SR_IBB))
> > + break;
> > if (signal_pending(current)) {
> > dev_dbg(&i2c_imx->adapter.dev,
> > "<%s> I2C Interrupted\n", __func__);
> > @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> > *i2c_imx)
> > return -EIO;
> > }
> > schedule();
> > + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> > }
> >
> > return 0;
> > @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct
> > *i2c_imx)
> > return 0;
> > }
> >
> > -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> > +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> > {
> > unsigned int temp = 0;
> > + int result;
> >
> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >
> > /* Enable I2C controller */
> > + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> > writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> > +
> > + result = i2c_imx_bus_busy(i2c_imx, 0);
> > + if (result)
> > + return result;
> > +
> > /* Start I2C transaction */
> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> > temp |= I2CR_MSTA;
> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> > + result = i2c_imx_bus_busy(i2c_imx, 1);
> > + if (result)
> > + return result;
> > +
> > temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> > + return result;
> > }
> >
> > static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> > @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct
> > *i2c_imx)
> > /* Stop I2C transaction */
> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> > - temp &= ~I2CR_MSTA;
> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> > - /* setup chip registers to defaults */
> > - writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> > - writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> > /*
> > * This delay caused by an i.MXL hardware bug.
> > * If no (or too short) delay, no "STOP" bit will be generated.
> > */
> > udelay(i2c_imx->disable_delay);
> > +
> > + i2c_imx_bus_busy(i2c_imx, 0);
> > +
> > /* Disable I2C controller */
> > writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> > }
> > @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx,
> > struct i2c_msg *msgs)
> > dev_dbg(&i2c_imx->adapter.dev,
> > "<%s> clear MSTA\n", __func__);
> > temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> > - temp &= ~I2CR_MSTA;
> > + temp &= ~(I2CR_MSTA | I2CR_MTX);
> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> > } else if (i == (msgs->len - 2)) {
> > dev_dbg(&i2c_imx->adapter.dev,
> > @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >
> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >
> > - /* Check if i2c bus is not busy */
> > - result = i2c_imx_bus_busy(i2c_imx);
> > + /* Start I2C transfer */
> > + result = i2c_imx_start(i2c_imx);
> > if (result)
> > goto fail0;
> >
> > - /* Start I2C transfer */
> > - i2c_imx_start(i2c_imx);
> > -
> > /* read/write data */
> > for (i = 0; i < num; i+
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao wrote:
> After START/RESTART, wait for busy bit to be set and
> after STOP, wait for busy bit to be clear.
>
> Signed-off-by: Richard Zhao
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 4afba3e..156cc95 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -125,14 +125,19 @@ struct imx_i2c_struct {
> /** Functions for IMX I2C adapter driver
> ***
> ***/
>
> -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
> +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> {
> unsigned long orig_jiffies = jiffies;
> + unsigned int temp;
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> - /* wait for bus not busy */
> - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> + while (1) {
> + if (for_busy && (temp & I2SR_IBB))
> + break;
> + if (!for_busy && !(temp & I2SR_IBB))
> + break;
> if (signal_pending(current)) {
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> I2C Interrupted\n", __func__);
> @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> *i2c_imx)
> return -EIO;
> }
> schedule();
> + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> }
>
> return 0;
> @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
> return 0;
> }
>
> -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> {
> unsigned int temp = 0;
> + int result;
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> /* Enable I2C controller */
> + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> +
> + result = i2c_imx_bus_busy(i2c_imx, 0);
> + if (result)
> + return result;
> +
> /* Start I2C transaction */
> temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> temp |= I2CR_MSTA;
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> + result = i2c_imx_bus_busy(i2c_imx, 1);
> + if (result)
> + return result;
> +
> temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> + return result;
> }
>
> static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> /* Stop I2C transaction */
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> - temp &= ~I2CR_MSTA;
> + temp &= ~(I2CR_MSTA | I2CR_MTX);
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> - /* setup chip registers to defaults */
> - writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> - writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> /*
> * This delay caused by an i.MXL hardware bug.
> * If no (or too short) delay, no "STOP" bit will be generated.
> */
> udelay(i2c_imx->disable_delay);
> +
> + i2c_imx_bus_busy(i2c_imx, 0);
> +
> /* Disable I2C controller */
> writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> }
> @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx,
> struct i2c_msg *msgs)
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> clear MSTA\n", __func__);
> temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> - temp &= ~I2CR_MSTA;
> + temp &= ~(I2CR_MSTA | I2CR_MTX);
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> } else if (i == (msgs->len - 2)) {
> dev_dbg(&i2c_imx->adapter.dev,
> @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> - /* Check if i2c bus is not busy */
> - result = i2c_imx_bus_busy(i2c_imx);
> + /* Start I2C transfer */
> + result = i2c_imx_start(i2c_imx);
> if (result)
> goto fail0;
>
> - /* Start I2C transfer */
> - i2c_imx_start(i2c_imx);
> -
> /* read/write data */
> for (i = 0; i < num; i++) {
> if (i) {
> @@ -386,6 +401,9 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> temp |= I2CR_RSTA;
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> + result
