Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>]

Hi Andy,

I like your patch and I just did some testing on it. Unluckily, it
doesn't solve the lock-up problem.

I haven't investigated any further but I suspect that on top of the
cases I observed when debugging this (interrupts after initialisation of
dev, easy to prove) there are more obscure cases in which interrupts are
generated in an unexpected manner after errors. The interrupt-driven
transfer state machine of the driver implicitly relies on the fact that
all updates of dev which are related to the same transfer are performed
between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I
decided it was safer to disable the block before releasing the mutex
when I wrote my patch.

That said, I think the sequencing at transfer initialisation is more
logical with your patch and I wonder if it is still worth applying. Any
other opinions on this?

Greetings,
  Christian

On Fri, Jun 07, 2013 at 12:30:01PM +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index c41ca63..a1d3d95 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 ic_con;
>  
> -	/* Disable the adapter */
> -	__i2c_dw_enable(dev, false);
> -
>  	/* set the slave (target) address */
>  	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
>  
> @@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	mutex_lock(&dev->lock);
>  	pm_runtime_get_sync(dev->dev);
>  
> +	ret = i2c_dw_wait_bus_not_busy(dev);
> +	if (ret < 0)
> +		goto done;
> +
> +	/* Disable the adapter */
> +	__i2c_dw_enable(dev, false);
> +
>  	INIT_COMPLETION(dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
> @@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
>  
> -	ret = i2c_dw_wait_bus_not_busy(dev);
> -	if (ret < 0)
> -		goto done;
> -
>  	/* start the transfers */
>  	i2c_dw_xfer_init(dev);
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates