From: Esben Haabendal <[email protected]>
Always update the stopped state when busy status have been checked.
This is identical to what was done before, with the exception of error
handling.
Without this change, some errors cause the stopped state to be left in
incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
i2c_imx_xfer().
Signed-off-by: Esben Haabendal <[email protected]>
---
drivers/i2c/busses/i2c-imx.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d86f152176a4..1db8e6790afc 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
return -EAGAIN;
}
- if (for_busy && (temp & I2SR_IBB))
+ if (for_busy && (temp & I2SR_IBB)) {
+ i2c_imx->stopped = 0;
break;
- if (!for_busy && !(temp & I2SR_IBB))
+ }
+ if (!for_busy && !(temp & I2SR_IBB)) {
+ i2c_imx->stopped = 1;
break;
+ }
if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
dev_dbg(&i2c_imx->adapter.dev,
"<%s> I2C bus is busy\n", __func__);
@@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
result = i2c_imx_bus_busy(i2c_imx, 1);
if (result)
return result;
- i2c_imx->stopped = 0;
temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
temp &= ~I2CR_DMAEN;
@@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
if (!i2c_imx->stopped) {
i2c_imx_bus_busy(i2c_imx, 0);
- i2c_imx->stopped = 1;
}
/* Disable I2C controller */
@@ -724,7 +726,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
temp &= ~(I2CR_MSTA | I2CR_MTX);
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
i2c_imx_bus_busy(i2c_imx, 0);
- i2c_imx->stopped = 1;
} else {
/*
* For i2c master receiver repeat restart operation like:
@@ -852,7 +853,6 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
temp &= ~(I2CR_MSTA | I2CR_MTX);
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
i2c_imx_bus_busy(i2c_imx, 0);
- i2c_imx->stopped = 1;
} else {
/*
* For i2c master receiver repeat restart operation like:
--
2.18.0
On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <[email protected]>
>
> Always update the stopped state when busy status have been checked.
> This is identical to what was done before, with the exception of error
> handling.
> Without this change, some errors cause the stopped state to be left in
> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
> i2c_imx_xfer().
>
> Signed-off-by: Esben Haabendal <[email protected]>
> ---
> drivers/i2c/busses/i2c-imx.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d86f152176a4..1db8e6790afc 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> return -EAGAIN;
> }
>
> - if (for_busy && (temp & I2SR_IBB))
> + if (for_busy && (temp & I2SR_IBB)) {
> + i2c_imx->stopped = 0;
> break;
> - if (!for_busy && !(temp & I2SR_IBB))
> + }
> + if (!for_busy && !(temp & I2SR_IBB)) {
> + i2c_imx->stopped = 1;
> break;
> + }
Would it make sense to assign to ->stopped independent of for_busy?
> if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> I2C bus is busy\n", __func__);
> @@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> result = i2c_imx_bus_busy(i2c_imx, 1);
> if (result)
> return result;
> - i2c_imx->stopped = 0;
>
> temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> temp &= ~I2CR_DMAEN;
> @@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>
> if (!i2c_imx->stopped) {
> i2c_imx_bus_busy(i2c_imx, 0);
> - i2c_imx->stopped = 1;
> }
The braces can go away here.
>
> /* Disable I2C controller */
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Uwe Kleine-König <[email protected]> writes:
> On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal <[email protected]>
>>
>> Always update the stopped state when busy status have been checked.
>> This is identical to what was done before, with the exception of error
>> handling.
>> Without this change, some errors cause the stopped state to be left in
>> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
>> i2c_imx_xfer().
>>
>> Signed-off-by: Esben Haabendal <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-imx.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index d86f152176a4..1db8e6790afc 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>> return -EAGAIN;
>> }
>>
>> - if (for_busy && (temp & I2SR_IBB))
>> + if (for_busy && (temp & I2SR_IBB)) {
>> + i2c_imx->stopped = 0;
>> break;
>> - if (!for_busy && !(temp & I2SR_IBB))
>> + }
>> + if (!for_busy && !(temp & I2SR_IBB)) {
>> + i2c_imx->stopped = 1;
>> break;
>> + }
>
> Would it make sense to assign to ->stopped independent of for_busy?
What do you mean?
Assigning to ->stopped on each check for I2SR_IBB in loop, independent
of the for_busy argument? I don't think so. The additional assignments
would be to the same value as it is set to already.
>> if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>> dev_dbg(&i2c_imx->adapter.dev,
>> "<%s> I2C bus is busy\n", __func__);
>> @@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>> result = i2c_imx_bus_busy(i2c_imx, 1);
>> if (result)
>> return result;
>> - i2c_imx->stopped = 0;
>>
>> temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>> temp &= ~I2CR_DMAEN;
>> @@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>>
>> if (!i2c_imx->stopped) {
>> i2c_imx_bus_busy(i2c_imx, 0);
>> - i2c_imx->stopped = 1;
>> }
>
> The braces can go away here.
Will do.
/Esben
On Thu, Aug 09, 2018 at 02:06:43PM +0200, Esben Haabendal wrote:
> Uwe Kleine-K?nig <[email protected]> writes:
>
> > On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
> >> From: Esben Haabendal <[email protected]>
> >>
> >> Always update the stopped state when busy status have been checked.
> >> This is identical to what was done before, with the exception of error
> >> handling.
> >> Without this change, some errors cause the stopped state to be left in
> >> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
> >> i2c_imx_xfer().
> >>
> >> Signed-off-by: Esben Haabendal <[email protected]>
> >> ---
> >> drivers/i2c/busses/i2c-imx.c | 12 ++++++------
> >> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> >> index d86f152176a4..1db8e6790afc 100644
> >> --- a/drivers/i2c/busses/i2c-imx.c
> >> +++ b/drivers/i2c/busses/i2c-imx.c
> >> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> >> return -EAGAIN;
> >> }
> >>
> >> - if (for_busy && (temp & I2SR_IBB))
> >> + if (for_busy && (temp & I2SR_IBB)) {
> >> + i2c_imx->stopped = 0;
> >> break;
> >> - if (!for_busy && !(temp & I2SR_IBB))
> >> + }
> >> + if (!for_busy && !(temp & I2SR_IBB)) {
> >> + i2c_imx->stopped = 1;
> >> break;
> >> + }
> >
> > Would it make sense to assign to ->stopped independent of for_busy?
>
> What do you mean?
>
> Assigning to ->stopped on each check for I2SR_IBB in loop, independent
> of the for_busy argument? I don't think so. The additional assignments
> would be to the same value as it is set to already.
Currently you have:
if (for_busy && (temp & I2SR_IBB)) {
i2c_imx->stopped = 0;
break;
}
if (!for_busy && !(temp & I2SR_IBB)) {
i2c_imx->stopped = 1;
break;
}
The semantic of this is the same (apart from always updating .stopped)
but is imho easier:
i2c_imx->stopped = !(temp & I2SR_IBB);
if (for_busy != i2c_imx->stopped)
break;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Uwe Kleine-König <[email protected]> writes:
> On Thu, Aug 09, 2018 at 02:06:43PM +0200, Esben Haabendal wrote:
>> Uwe Kleine-König <[email protected]> writes:
>>
>> > On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
>> >> From: Esben Haabendal <[email protected]>
>> >>
>> >> Always update the stopped state when busy status have been checked.
>> >> This is identical to what was done before, with the exception of error
>> >> handling.
>> >> Without this change, some errors cause the stopped state to be left in
>> >> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
>> >> i2c_imx_xfer().
>> >>
>> >> Signed-off-by: Esben Haabendal <[email protected]>
>> >> ---
>> >> drivers/i2c/busses/i2c-imx.c | 12 ++++++------
>> >> 1 file changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> >> index d86f152176a4..1db8e6790afc 100644
>> >> --- a/drivers/i2c/busses/i2c-imx.c
>> >> +++ b/drivers/i2c/busses/i2c-imx.c
>> >> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>> >> return -EAGAIN;
>> >> }
>> >>
>> >> - if (for_busy && (temp & I2SR_IBB))
>> >> + if (for_busy && (temp & I2SR_IBB)) {
>> >> + i2c_imx->stopped = 0;
>> >> break;
>> >> - if (!for_busy && !(temp & I2SR_IBB))
>> >> + }
>> >> + if (!for_busy && !(temp & I2SR_IBB)) {
>> >> + i2c_imx->stopped = 1;
>> >> break;
>> >> + }
>> >
>> > Would it make sense to assign to ->stopped independent of for_busy?
>>
>> What do you mean?
>>
>> Assigning to ->stopped on each check for I2SR_IBB in loop, independent
>> of the for_busy argument? I don't think so. The additional assignments
>> would be to the same value as it is set to already.
>
> Currently you have:
>
> if (for_busy && (temp & I2SR_IBB)) {
> i2c_imx->stopped = 0;
> break;
> }
>
> if (!for_busy && !(temp & I2SR_IBB)) {
> i2c_imx->stopped = 1;
> break;
> }
>
> The semantic of this is the same (apart from always updating .stopped)
> but is imho easier:
>
> i2c_imx->stopped = !(temp & I2SR_IBB);
>
> if (for_busy != i2c_imx->stopped)
> break;
Yes, that should work also.
Shorter, but IMHO a bit more convoluted to read.
Let me know if I should send a new version with this change.
/Esben
Hello Esben,
On Fri, Aug 10, 2018 at 11:25:34AM +0200, Esben Haabendal wrote:
> Uwe Kleine-K?nig <[email protected]> writes:
> > Currently you have:
> >
> > if (for_busy && (temp & I2SR_IBB)) {
> > i2c_imx->stopped = 0;
> > break;
> > }
> >
> > if (!for_busy && !(temp & I2SR_IBB)) {
> > i2c_imx->stopped = 1;
> > break;
> > }
> >
> > The semantic of this is the same (apart from always updating .stopped)
> > but is imho easier:
> >
> > i2c_imx->stopped = !(temp & I2SR_IBB);
> >
> > if (for_busy != i2c_imx->stopped)
> > break;
>
> Yes, that should work also.
> Shorter, but IMHO a bit more convoluted to read.
> Let me know if I should send a new version with this change.
unless someone else chimes in I'd say keep it as is. I'd prefer my
variant, but I accept that this is something subjective.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |