[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote:
>> On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote:
>> >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
>> >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
>> >> > transaction) during a DATA or WAIT phase.  In other words, the
>> >> > controller rejects a STOP requested as part of the first transaction in a
>> >> > sequence.
>> >> >
>> >> > Thus, for the first transaction we must always use a WAIT cycle, detect
>> >> > when the device has finished (and is in a WAIT phase), and then either
>> >> > start the next transaction, or, if there are no more transactions,
>> >> > generate a STOP cycle.
>> >> >
>> >> > Note: Theoretically, the last transaction of a multi-transaction sequence
>> >> > could initiate a STOP cycle.  However, this slight optimization is left
>> >> > for another patch.  We return -ETIMEDOUT if the hardware doesn't
>> >> > deactivate after the STOP cycle.
>> >> >
>> >> > Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
>> >>
>> >> I've re-read gmbus register spec and STOP seems to be allowed even in the
>> >> first cycle. Does this patch solve an issue for you? If not, I prefer we
>> >> just drop it.
>>
>> STOP does not work in the first cycle, hence the patch.
>
> Ok, I've picked this patch up and extended the comment a bit to that
> effect. Just to avoid anyone else trying to 'fix' things because bspec
> sounds like it should work.
>
> I've also picked up the other patches safe for the last one, thanks a lot
> for digging through the gmbus code and fixing it all up.
>
> Now can I volunteer you for a (hopefully) last set of gmbus patches?
> Afaics there are a few small things left to fix:
> - zero-length reads can blow up the kernel, like zero-length writes could.
>  See: https://bugs.freedesktop.org/show_bug.cgi?id=48269

Got it.  Will Fix.

> - Chris Wilson suggested on irc that we should wait for HW_READY even for
>  zero-length writes (and also reads), currently we don't.

I don't think so.  We just need to wait for (GMBUS_SATOER |
GMBUS_HW_WAIT_PHASE).
Why would we wait for HW_READY, too?

> - atm the debug output is too noisy. I think we can leave the fallback to
>  gpio bitbanging at info (or maybe error) level, but all the other
>  messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit
>  when userspace tries to probe the i2c with nothing connected or if the
>  driver code tries to do the same. See:
>  https://bugs.freedesktop.org/show_bug.cgi?id=48248

OK...  we can change the logging level.
However, the log in the bug to which you link seems to indicate a more
serious issue in this case.  It says to me that something on his
system is trying to talk to the disabled dpc i2c port 5 times every 10
seconds.  Each time it fails due with a time out, and each timeout
takes 50ms.  I would argue that the INFO message here is pointing out
that the hotplug code might want to check the corresponding
PORT_ENABLED bit before attempting a read over a particular DP/HDMI
gmbus port.  Perhaps I am mistaken, but if there was really nothing on
the bus, shouldn't that be a NAK, not a timeout?

>
> Chris, anything you want to add to the wishlist?
>
> Thanks, Daniel
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48