Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for asequence of requests.

[Bryan Freed]

Hi Andi,

On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <andi.shyti@xxxxxxxxx> 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.

> Andi

bryan.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/