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

["Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org" <grygorii.strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>   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

-- 
regards,
-grygorii