Messages in this thread | | | Date | Wed, 2 May 2012 22:18:32 +0200 | From | Andi Shyti <> | Subject | Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for a sequence of requests. |
| |
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. 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
| |