[2/3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

There are several problems in the function:
- "to_cnt" variable does nothing
- schedule_timeout() call without setting current state does nothing
- "allow_sleep" parameter is not really used

Refactor the function so that it really tries to wait. In case of timeout try
to recover the bus and finally initialize the controller. We cannot really
do more than that, so it makes no sense to check BB bit after init.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/i2c/busses/i2c-davinci.c |   53 +++++++++++++++++++++----------------
 1 files changed, 30 insertions(+), 23 deletions(-)

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

Hi Alexander,

On 03/11/2015 03:08 PM, Alexander Sverdlin wrote:
> There are several problems in the function:
> - "to_cnt" variable does nothing
> - schedule_timeout() call without setting current state does nothing
> - "allow_sleep" parameter is not really used
>
> Refactor the function so that it really tries to wait. In case of timeout try
> to recover the bus and finally initialize the controller. We cannot really
> do more than that, so it makes no sense to check BB bit after init.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>   drivers/i2c/busses/i2c-davinci.c |   53 +++++++++++++++++++++----------------
>   1 files changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 98759ae..ca22479 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -152,6 +152,26 @@ static void davinci_i2c_clock_pulse(unsigned int scl_pin)
>   	}
>   }
>
> +/*
> + * Wait until specific bit in status register has particular value
> + * Function returns 0 if condition was met,
> + * -ETIMEDOUT in case of timeout.
> + */
> +static int i2c_davinci_wait_status_change(struct davinci_i2c_dev *dev, u16 mask,
> +                                          u16 val)

do we really need it as separate function? (even if it looks good:)

> +{
> +	unsigned long timeout = jiffies + dev->adapter.timeout;
> +
> +	while ((davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) & mask) != val) {
> +		if (time_after(jiffies, timeout))
> +			return -ETIMEDOUT;
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		schedule_timeout(1);

schedule_timeout_uninterruptible()

> +	}
> +
> +	return 0;
> +}
> +
>   /* This routine does i2c bus recovery as specified in the
>    * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>    */
> @@ -269,29 +289,16 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>   /*
>    * Waiting for bus not busy
>    */
> -static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
> -					 char allow_sleep)
> +static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev)
>   {
> -	unsigned long timeout;
> -	static u16 to_cnt;
> -
> -	timeout = jiffies + dev->adapter.timeout;
> -	while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
> -	       & DAVINCI_I2C_STR_BB) {
> -		if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
> -			if (time_after(jiffies, timeout)) {
> -				dev_warn(dev->dev,
> -				"timeout waiting for bus ready\n");
> -				to_cnt++;
> -				return -ETIMEDOUT;
> -			} else {
> -				to_cnt = 0;
> -				davinci_i2c_recover_bus(dev);
> -				i2c_davinci_init(dev);
> -			}
> -		}
> -		if (allow_sleep)
> -			schedule_timeout(1);
> +	if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
> +		dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +		davinci_i2c_recover_bus(dev);
> +		i2c_davinci_init(dev);
> +		/*
> +		 * the bus should not be busy after init, otherwise something
> +		 * is badly broken
> +		 */

I think you should recheck BB and return error if it's still detected.

>   	}
>
>   	return 0;
> @@ -449,7 +456,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>
>   	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
>
> -	ret = i2c_davinci_wait_bus_not_busy(dev, 1);
> +	ret = i2c_davinci_wait_bus_not_busy(dev);
>   	if (ret < 0) {
>   		dev_warn(dev->dev, "timeout waiting for bus ready\n");
>   		return ret;
>


regards,
-grygorii

Hi!

Grygorii.Strashko@... <grygorii.strashko@...> writes:
> > +/*
> > + * Wait until specific bit in status register has particular value
> > + * Function returns 0 if condition was met,
> > + * -ETIMEDOUT in case of timeout.
> > + */
> > +static int i2c_davinci_wait_status_change(struct davinci_i2c_dev *dev, 
u16 mask,
> > +                                          u16 val)
> 
> do we really need it as separate function? (even if it looks good:)

Initially I had a patch for "recover" function also, but I've dropped it and 
I'm going to test your series. It might be re-used in case if we want to 
wait for something in the recover...
 
> > +{
> > +	unsigned long timeout = jiffies + dev->adapter.timeout;
> > +
> > +	while ((davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) & mask) != 
val) {
> > +		if (time_after(jiffies, timeout))
> > +			return -ETIMEDOUT;
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		schedule_timeout(1);
> 
> schedule_timeout_uninterruptible()

Will do in v2...

[...]

> > +	if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
> > +		dev_warn(dev->dev, "timeout waiting for bus ready\n");
> > +		davinci_i2c_recover_bus(dev);
> > +		i2c_davinci_init(dev);
> > +		/*
> > +		 * the bus should not be busy after init, otherwise 
something
> > +		 * is badly broken
> > +		 */
> 
> I think you should recheck BB and return error if it's still detected.

Good idea! Will do in v2...

> >   	}
> >
> >   	return 0;
> >  <at>  <at>  -449,7 +456,7  <at>  <at>  i2c_davinci_xfer(struct 
i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >
> >   	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> >
> > -	ret = i2c_davinci_wait_bus_not_busy(dev, 1);
> > +	ret = i2c_davinci_wait_bus_not_busy(dev);
> >   	if (ret < 0) {
> >   		dev_warn(dev->dev, "timeout waiting for bus ready\n");
> >   		return ret;


Alex.


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

Hi!

On 11/03/15 19:35, ext Grygorii.Strashko@linaro.org wrote:
>> +    if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
>> +        dev_warn(dev->dev, "timeout waiting for bus ready\n");
>> +        davinci_i2c_recover_bus(dev);
>> +        i2c_davinci_init(dev);
>> +        /*
>> +         * the bus should not be busy after init, otherwise something
>> +         * is badly broken
>> +         */
> 
> I think you should recheck BB and return error if it's still detected.

But after re-reading the datasheet, I must conclude, this is almost not possible.
It will be a dead code. Reset will clear BB anyway and the only possibility to see
BB set to 1 here is when another master transmits right after our reset. We cannot
guarantee that we will always catch this here, so this must be any way handled over
AL interrupt.

On 12 March 2015 at 10:50, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> On 11/03/15 19:35, ext Grygorii.Strashko@linaro.org wrote:
>>> +    if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
>>> +        dev_warn(dev->dev, "timeout waiting for bus ready\n");
>>> +        davinci_i2c_recover_bus(dev);
>>> +        i2c_davinci_init(dev);
>>> +        /*
>>> +         * the bus should not be busy after init, otherwise something
>>> +         * is badly broken
>>> +         */
>>
>> I think you should recheck BB and return error if it's still detected.
>
> But after re-reading the datasheet, I must conclude, this is almost not possible.
> It will be a dead code. Reset will clear BB anyway and the only possibility to see
> BB set to 1 here is when another master transmits right after our reset. We cannot
> guarantee that we will always catch this here, so this must be any way handled over
> AL interrupt.

The question here what is worse:
- silently continue execution with undefined behavior
- or recheck and return error (That's how it was before).

I like option 2 :) - don't believe HW.

Hi!

On 12/03/15 13:37, ext Grygorii Strashko wrote:
>>>> +    if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
>>>> >>> +        dev_warn(dev->dev, "timeout waiting for bus ready\n");
>>>> >>> +        davinci_i2c_recover_bus(dev);
>>>> >>> +        i2c_davinci_init(dev);
>>>> >>> +        /*
>>>> >>> +         * the bus should not be busy after init, otherwise something
>>>> >>> +         * is badly broken
>>>> >>> +         */
>>> >>
>>> >> I think you should recheck BB and return error if it's still detected.
>> >
>> > But after re-reading the datasheet, I must conclude, this is almost not possible.
>> > It will be a dead code. Reset will clear BB anyway and the only possibility to see
>> > BB set to 1 here is when another master transmits right after our reset. We cannot
>> > guarantee that we will always catch this here, so this must be any way handled over
>> > AL interrupt.
> The question here what is worse:
> - silently continue execution with undefined behavior
> - or recheck and return error (That's how it was before).
> 
> I like option 2 :) - don't believe HW.

OK, it will at least catch the cases when the bus is short-circuited.