From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 18 Mar 2016 12:12:29 +0100
Subject: [U-Boot] [PATCH 2/6] i2c: designware_i2c: Add dw_i2c_enable()
	helper function
In-Reply-To: <1458287661-21745-2-git-send-email-sr@denx.de>
References: <1458287661-21745-1-git-send-email-sr@denx.de>
 <1458287661-21745-2-git-send-email-sr@denx.de>
Message-ID: <56EBE29D.8050807@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/18/2016 08:54 AM, Stefan Roese wrote:
> dw_i2c_enable() is used to dis-/en-able the I2C controller. It makes
> sense to add such a function, as the controller is dis-/en-abled
> multiple times in the code. Additionally, this function now checks,
> if the controller is really dis-/en-abled. This code is copied
> from the Linux I2C driver version.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>  drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> index e768cde..c8ea520 100644
> --- a/drivers/i2c/designware_i2c.c
> +++ b/drivers/i2c/designware_i2c.c
> @@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)
>  	return NULL;
>  }
>  
> +static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)
> +{
> +	int timeout = 100;
> +
> +	do {
> +		writel(enable, &i2c_base->ic_enable);

This should at least use IC_ENABLE_0B and not the boot enable.

> +		if ((readl(&i2c_base->ic_enable_status) & 1) == enable)
> +			return;
> +
> +		/*
> +		 * Wait 10 times the signaling period of the highest I2C
> +		 * transfer supported by the driver (for 400KHz this is
> +		 * 25us) as described in the DesignWare I2C databook.
> +		 */
> +		udelay(25);
> +	} while (timeout--);
> +
> +	printf("timeout in %sabling I2C adapter\n", enable ? "en" : "dis");
> +}
> +
>  /*
>   * set_speed - Set the i2c speed mode (standard, high, fast)
>   * @i2c_spd:	required i2c speed mode
> @@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)
>  	struct i2c_regs *i2c_base = i2c_get_base(adap);
>  	unsigned int cntl;
>  	unsigned int hcnt, lcnt;
> -	unsigned int enbl;
>  
>  	/* to set speed cltr must be disabled */
> -	enbl = readl(&i2c_base->ic_enable);
> -	enbl &= ~IC_ENABLE_0B;
> -	writel(enbl, &i2c_base->ic_enable);
> +	dw_i2c_enable(i2c_base, 0);

This and all the other places which you changed actually change the
logic of the code, right ? Is that a problem ?

Best regards,
Marek Vasut