From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 18 Mar 2016 13:14:55 +0100
Subject: [U-Boot] [PATCH 2/6] i2c: designware_i2c: Add dw_i2c_enable()
	helper function
In-Reply-To: <56EBEEC3.8010107@denx.de>
References: <1458287661-21745-1-git-send-email-sr@denx.de>
 <1458287661-21745-2-git-send-email-sr@denx.de> <56EBE29D.8050807@denx.de>
 <56EBEEC3.8010107@denx.de>
Message-ID: <56EBF13F.9020203@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 01:04 PM, Stefan Roese wrote:
> 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. ;)

I will have a board ready for mainlining that uses i2c, so if something
breaks, I will cry ;-)

-- 
Best regards,
Marek Vasut