| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <546F9382.40102@ti.com>
Date: Fri, 21 Nov 2014 21:33:22 +0200
From: Grygorii Strashko <grygorii.strashko@...com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
CC: Wolfram Sang <wsa@...-dreams.de>, <linux-i2c@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Sekhar Nori <nsekhar@...com>,
Kevin Hilman <khilman@...prootsystems.com>,
Santosh Shilimkar <ssantosh@...nel.org>,
Murali Karicheri <m-karicheri2@...com>
Subject: Re: [4/5] i2c: davinci: use bus recovery infrastructure
Hi Uwe,
On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
>> This patch converts Davinci I2C driver to use I2C bus recovery
>> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
>> bus recovery infrastructure").
>>
>> The i2c_bus_recovery_info is configured for Davinci I2C adapter
>> only in case if scl_pin is provided in Platform data at least.
> s/Platform/platform/
>
>>
>> Because the controller must be held in reset while doing so, the
> s/Because/As/
>
>> recovery routine must re-init the controller. Since this was already
>> being done after each call to i2c_recover_bus, move those calls into
>> the recovery_prepare/unprepare routines and as well.
>>
>> CC: Sekhar Nori <nsekhar@...com>
>> CC: Kevin Hilman <khilman@...prootsystems.com>
>> CC: Santosh Shilimkar <ssantosh@...nel.org>
>> CC: Murali Karicheri <m-karicheri2@...com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++----------------------
>> 1 file changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 2cef115..db2a2cd 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>> return readw_relaxed(i2c_dev->base + reg);
>> }
>>
>> -/* Generate a pulse on the i2c clock pin. */
>> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
>> -{
>> - u16 i;
>> -
>> - if (scl_pin) {
>> - /* Send high and low on the SCL line */
>> - for (i = 0; i < 9; i++) {
>> - gpio_set_value(scl_pin, 0);
>> - udelay(20);
>> - gpio_set_value(scl_pin, 1);
>> - udelay(20);
>> - }
>> - }
>> -}
>> -
>> -/* This routine does i2c bus recovery as specified in the
>> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> - */
>> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
>> -{
>> - u32 flag = 0;
>> - struct davinci_i2c_platform_data *pdata = dev->pdata;
>> -
>> - dev_err(dev->dev, "initiating i2c bus recovery\n");
>> - /* Send NACK to the slave */
>> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> - flag |= DAVINCI_I2C_MDR_NACK;
>> - /* write the data into mode register */
>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> - davinci_i2c_clock_pulse(pdata->scl_pin);
>> - /* Send STOP */
>> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> - flag |= DAVINCI_I2C_MDR_STP;
>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> -}
>> -
>> static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>> int val)
>> {
>> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>> return 0;
>> }
>>
>> +/* This routine does i2c bus recovery as specified in the
>> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> + */
> This comment is wrong. The actual bus clear is implemented by
> i2c_generic_gpio_recovery. Also while touching this comment, convert it
> to the usual format with /* on its own line. (The file in question has
> already both types of comment, so consistency is not a reason to keep it
> as is.)
It has been just copy-pasted, but ok.
I'll change this comment as following:
/*
* This routine does i2c bus recovery by using i2c_generic_gpio_recovery
* which is provided by I2C Bus recovery infrastructure.
*/
Is it ok?
>
> Even though I remember that I reviewed this bus recovery patch (that
> resulted in 5f9296ba21b3) back then, I don't remember why it was split
> in prepare + recover + unprepare. But that is unrelated to this patch.
>
>> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
>> +{
>> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> + dev_err(dev->dev, "initiating i2c bus recovery\n");
>> + /* Disable interrupts */
>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
>> +
>> + /* put I2C into reset */
>> + davinci_i2c_reset_ctrl(dev, 0);
>> +}
>> +
>> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
>> +{
>> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> + i2c_davinci_init(dev);
>> +}
>> +
>> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
> I'd call this only davinci_i2c_recovery_info.
No. Pls, see next patch.
>
>> + .recover_bus = i2c_generic_gpio_recovery,
>> + .prepare_recovery = davinci_i2c_prepare_recovery,
>> + .unprepare_recovery = davinci_i2c_unprepare_recovery,
>> +};
> new line here please
:) Ok.
Fixed in next patch.
>
>> /*
>> * Waiting for bus not busy
>> */
>> @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>> return -ETIMEDOUT;
>> } else {
>> to_cnt = 0;
>> - davinci_i2c_recover_bus(dev);
>> - i2c_davinci_init(dev);
>> + i2c_recover_bus(&dev->adapter);
>> }
>> }
>> if (allow_sleep)
>> @@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>> dev->adapter.timeout);
>> if (r == 0) {
>> dev_err(dev->dev, "controller timed out\n");
>> - davinci_i2c_recover_bus(dev);
>> - i2c_davinci_init(dev);
>> + i2c_recover_bus(adap);
>> dev->buf_len = 0;
>> return -ETIMEDOUT;
>> }
>> @@ -717,6 +705,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> adap->timeout = DAVINCI_I2C_TIMEOUT;
>> adap->dev.of_node = pdev->dev.of_node;
>>
>> + if (dev->pdata->scl_pin) {
>> + adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
>> + adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
>> + adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
>> + }
>> +
>> adap->nr = pdev->id;
>> r = i2c_add_numbered_adapter(adap);
>> if (r) {
> Just another general comment about the driver that doesn't influence the
> correctness of this patch: The i2c-davinci driver is quite quick to
> reset the bus. I wonder how often this reset triggers. Is the bus in
> question less "stable" than others?
In comparison to ..? :)
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists