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

[Andi Shyti]

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@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.

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
--
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/