Re: [PATCH] drm/bridge: sii902x: fix get edid may fail

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 23 Jan 2017 13:12:12 +0100
Andrea Merello <andrea.merello@gmail.com> wrote:

> On Mon, Jan 23, 2017 at 12:32 PM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Mon, 23 Jan 2017, Boris Brezillon <boris.brezillon@free-electrons.com> wrote:  
> >> Hi Andrea,
> >>
> >> On Mon, 23 Jan 2017 11:00:02 +0100
> >> Andrea Merello <andrea.merello@gmail.com> wrote:
> >>  
> >>> From: Andrea Merello <andrea.merello@gmail.com>
> >>>
> >>> The standard DRM function to get the edid from the i2c bus performs
> >>> (at least) two transfers.
> >>>
> >>> By experiments it seems that the sii9022a have problems with the
> >>> 2nd I2C start, at least unless a wait is introduced detween the  
> >>
> >>                                                     ^ between
> >>  
> >>> two transfers.
> >>>
> >>> So we perform one single I2C transfer, and if the transfer must be
> >>> split, then we introduce a delay.  
> >>
> >> That's not exactly what this patch does: you're introducing a delay
> >> between each retry. So, if the transceiver really requires a delay
> >> between each transfer, you'll have to retry at least once on the 2nd
> >> transfer.
> >>
> >> I guess a better solution would be to add a delay even in case of
> >> success, or maybe modify drm_do_get_edid() to optionally wait for a
> >> specified time between each transfer.  
> >
> > Is the problem related specifically to EDID reads, or generally to I2C
> > transfers? Perhaps this should be fixed at the adapter master_xfer layer
> > instead? Does the master_xfer perhaps return -EAGAIN, have you looked
> > into i2c_adapter timeout member? (See __i2c_transfer in i2c-core.c.)
> >
> > The intention of drm_do_get_edid() is to facilitate *alternative*
> > methods to doing I2C, not to workaround quirks like this.

Yes, I tend to agree with that.

> 
> This problem seems not related to the I2C master itself. I saw the I2C
> master to TX correctly by looking at the I2C bus before the sii9022a
> with the scope, and I saw the same I2C transfer to be corrupted after
> the sii9022a; actually the sii9022a seems to gate the bus damaging the
> I2C start of the 2nd transfer, unless we place this delay.
> 
> This happened with two different I2C master (the one on the SoC as
> well as another different one implemented on FPGA).

Actually, I faced the same problem on an at91-based platform, and came
up with pretty much the same fix (add a delay after each transfer),
except mine was uglier and I never took the time to clean it up and
submit it.

> 
> So IMHO this quirk should be done in the sii902x dirver. Said that, I
> admit that I've not looked at i2c_adapter timeout member or other
> details in upper layers.

Well, let's sum up the situation: the sii902x device is acting both as
a regular i2c device and as an i2c 'pass-through' device. In the
pass-through mode, the si902x has a new constraint: the i2c transfers
have to be separated by an idle period.

Maybe we should expose the pass-through mode as separate i2c adapter,
that would be registered when entering pass-through mode and
de-registered when leaving this mode.
This way, we can implement the ->master_xfer() as we need (add a delay
after each xfer), and still use the generic drm_get_edid().

What do you think?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel