[U-Boot] [PATCH 2/6] i2c: designware_i2c: Add dw_i2c_enable() helper function

[Stefan Roese <sr@denx.de>]

On 18.03.2016 12:12, Marek Vasut wrote:
> 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 ?

It is a functional change, yes. With a now added check, if the
controller is actually getting enabled or disabled. The code is
taken from the Linux kernel, as noted in the commit text. And
I've tested this code on SoCFPGA without any issues so far.

Additional testing would be very welcome though. ;)

Thanks,
Stefan