Date: Mon, 20 Feb 2012 14:30:40 +0100
From: Daniel Vetter <daniel@ffwll.ch>
To: Daniel Kurtz <djkurtz@chromium.org>
Cc: Benson Leung <bleung@chromium.org>, keithp@keithp.com, airlied@linux.ie,
        chris@chris-wilson.co.uk, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes
Message-ID: <20120220133040.GC4104@phenom.ffwll.local>
Mail-Followup-To: Daniel Kurtz <djkurtz@chromium.org>,
	Benson Leung <bleung@chromium.org>, keithp@keithp.com,
	airlied@linux.ie, chris@chris-wilson.co.uk,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
References: <1328817797-4026-1-git-send-email-bleung@chromium.org>
 <20120215104837.GB4095@phenom.ffwll.local>
 <CAGS+omD8e9dBen_kQkoY+vc4yP6HCAeoNRYhzK+OPw+99HDoFg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGS+omD8e9dBen_kQkoY+vc4yP6HCAeoNRYhzK+OPw+99HDoFg@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2585
Lines: 57

On Mon, Feb 20, 2012 at 07:22:00PM +0800, Daniel Kurtz wrote:
> On Feb 15, 2012 6:48 PM, "Daniel Vetter" <daniel@ffwll.ch> wrote:
> >
> > On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> > > gmbus_xfer with a single message (particularly a single message write) would
> > > set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> > > No Index, Stop cycle. This would not start single message i2c transactions.
> > >
> > > Also, gmbus_xfer done: will disable the interface without checking if
> > > it is idle. In the case of writes, there will be no wait on status or delay
> > > to ensure the write starts and completes before the interface is turned off.
> > >
> > > Fixed the former issue by using the same cycle selection as used in the
> > > I2C_M_RD for the write case.
> > > GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> > > Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> > >
> > > Signed-off-by: Benson Leung <bleung@chromium.org>
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> >
> > Can you clarify the commit message a bit and say that the first hunk is
> > just for optics and the issue is only with the write path (because the
> > read path is correct already). Silly me is just to easily confused ;-)
> >
> > Btw, I've reworked the gmbus -> gpio bit-banging fallback code a bit and
> > if that passes review and all I'll reenable gmbus by default again. See
> >
> > http://cgit.freedesktop.org/~danvet/drm/log/?h=gmbus
> 
> If the write case is fixed by Benson's patch, is there any known use
> case that still requires i2c bit banging on these pins?  It would be a
> nice cleanup to remove it completely.

Let's first see how much things blow up when re-enabling gmbus again ;-)

> Also, I can think of at least two further potential performance
> improvements that I was wondering if anybody has yet pursued:
>   (1) Enabling the i915's gmbus interrupt.  This would eliminate the
> need for the (relatively slow) wait_for polling loop.
>   (2) Taking advantage of the i915's "INDEX" cycles to combine writing
> a (1 or 2 byte) address & reading back an array of bytes into a single
> transaction.

Afaik no one looked into this, but patches are highly welcome.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
--
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/