Messages in this thread Patch in this message | | | Date | Wed, 2 May 2012 14:58:45 -0700 | Subject | Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for a sequence of requests. | From | Bryan Freed <> |
| |
Andi,
On Wed, May 2, 2012 at 1:18 PM, Andi Shyti <andi.shyti@gmail.com> wrote: > Hi, > > On Wed, May 02, 2012 at 12:06:14PM -0700, Bryan Freed wrote: >> Hi Andi, >> >> On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <andi.shyti@gmail.com> wrote: >> > Hi Bryan, >> > >> >> > try to have a look to the i2c_smbus* function, you could avoid >> >> > lot of code >> > On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote: >> >> (Sorry for resending...) >> >> >> >> Andi, it is not clear what i2c_smbus_* functions in particular will >> >> improve upon this change. >> >> >> >> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some >> >> point will i2c_lock_adapter() for each request. >> >> This is true for adapters that support SMBUS (where the lock occurs >> >> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in >> >> i2c_transfer() called through i2c_smbus_xfer_emulated()). >> >> >> >> The goal of this change is for the tpm_tis_i2c driver to be able to >> >> lock an adapter over the duration of several I2C requests. >> >> Presumably, that is why i2c_lock_adapter() is exported. >> > >> > the i2c_smbus_* functions will not improve anything to the >> > driver, it will increase the readability and it will allow you to >> > do the same stuff with less code. >> >> I think I see what you are saying. >> We could (in the unmodified version of this driver) replace all the >> iic_tpm_read() calls of one byte (which sends an address byte before >> reading a data byte) with an i2c_smbus_read_byte_command() call which >> does the same thing. >> Switching to the SMBUS calls in this driver will still work on >> adapters that only support I2C because of i2c_smbus_xfer_emulated(). >> Right? >> > All the i2c communication implementation you wrote here, is >> > already written in the i2c-core.c file. >> >> Right. Unfortunately, we cannot use any of the i2c_smbus_* functions >> in this driver. The device will fail because the adapter lock is not >> held long enough to prevent I2C traffic going to other devices on the >> same bus. That other traffic to other devices confuses the i2c core >> in this device. Our only driver solution is to lock the adapter for a >> longer duration. >> >> Yes, the code we have here is copied from the i2c-core.c file. In >> fact, we comment this with, "Copy i2c-core:i2c_transfer() as close as >> possible without the adapter locks >> and algorithm check". >> >> And that really is the problem with using the i2c-core.c calls. This >> driver needs to lock the adapter for an extended duration. > > mmhhh... you still haven't convinced me. I always thought that > every dublicated code is useless.
Hey, I agree with you on that point. Duplicated code has its own problems. A better solution would require i2c-core.c mods, mainly:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index e9c1893..64cb9c2 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1346,17 +1346,8 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) i2c_lock_adapter(adap); }
- /* Retry automatically on arbitration loss */ - orig_jiffies = jiffies; - for (ret = 0, try = 0; try <= adap->retries; try++) { - ret = adap->algo->master_xfer(adap, msgs, num); - if (ret != -EAGAIN) - break; - if (time_after(jiffies, orig_jiffies + adap->timeout)) - break; - } + ret = i2c_transfer_nolock(&adap-, msgs, num); i2c_unlock_adapter(adap); - return ret; } else { dev_dbg(&adap->dev, "I2C level transfers not supported\n"); @@ -1365,6 +1356,25 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) } EXPORT_SYMBOL(i2c_transfer);
+int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +{ + unsigned long orig_jiffies; + int ret, try; + + /* Retry automatically on arbitration loss */ + orig_jiffies = jiffies; + for (ret = 0, try = 0; try <= adap->retries; try++) { + ret = adap->algo->master_xfer(adap, msgs, num); + if (ret != -EAGAIN) + break; + if (time_after(jiffies, orig_jiffies + adap->timeout)) + break; + } + + return ret; +} +EXPORT_SYMBOL(i2c_transfer_nolock); +
Then I would not need my own copy of i2c_transfer_nolock(). But making these ugly changes in the driver for one buggy device is easier/safer than making it in the general I2C code.
bryan.
> You may have good reasons to do that, but I could still try to > find out a way on how to simplify it. > Thanks for the explanation, > Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |