Date: Wed, 2 May 2012 22:18:32 +0200
From: Andi Shyti <andi.shyti@gmail.com>
To: Bryan Freed <bfreed@chromium.org>
Cc: Peter Huewe <peter.huewe@infineon.com>,
        Rajiv Andrade <srajiv@linux.vnet.ibm.com>, tpmdd@selhorst.net,
        tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        Olof Johansson <olof@lixom.net>,
        Luigi Semenzato <semenzato@google.com>
Subject: Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for a
 sequence of requests.
Message-ID: <20120502201832.GA19349@andi>
Mail-Followup-To: Bryan Freed <bfreed@chromium.org>,
	Peter Huewe <peter.huewe@infineon.com>,
	Rajiv Andrade <srajiv@linux.vnet.ibm.com>, tpmdd@selhorst.net,
	tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
	Olof Johansson <olof@lixom.net>,
	Luigi Semenzato <semenzato@google.com>
References: <1335967293-7728-1-git-send-email-peter.huewe@infineon.com>
 <1335967416-7564-1-git-send-email-peter.huewe@infineon.com>
 <20120502151734.GD16981@andi>
 <CAEYUcX0jBHasvcdLefR0fOGrVPaDw3pqzfQ_8_JGXHSPQHW7Ew@mail.gmail.com>
 <20120502181435.GF16981@andi>
 <CAEYUcX06hvTb1YP=9vCpK9Z19dtZgNTjUcSRnrUvjc3bu-D--Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEYUcX06hvTb1YP=9vCpK9Z19dtZgNTjUcSRnrUvjc3bu-D--Q@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2915
Lines: 68

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