[RFC] drm/bridge/sii902x: Fix EDID readback

[Peter Rosin]

On 2018-11-01 17:04, Fabrizio Castro wrote:
> Hello Peter,
> 
> Thank you for your feedback!
> 
>>> The "mux-locked" implementation was the one I first tried and I discovered
>>> it doesn't work for me, as other traffic on the parent adapter can get in the
>>> way. What we need for this device is no other traffic on the bus during the
>>> "select transaction deselect" procedure, that's why I went with the parent
>>> locking. Also this device needs a delay between stop and start conditions
>>> while addressing the monitor.
>>
>> Ok, I thought the problem was that a delay was needed between the STOP
>> of the command opening the gate and the START of the edid eeprom xfer, and
>> that everything else worked normally. Too bad this wasn't the actual problem.
>>
>> Hmm. If it is indeed true that other xfers must never creep into the "select
>> xfer deselect" procedure then you are indeed stuck with a parent-locking.
>> But is that really the case? Could it be that the extra delay between
>> STOP-START is only needed after the absolutely last xfer before the
>> command closing the gate?
>>
>> If that problem description is correct, it should be possible to go back to
>> a mux-locked gate, but then in your deselect implementation grab the i2c adapter
>> lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the
>> 30 us delay, then open code the command to close the gate with an unlocked i2c
>> access, and finally release the i2c bus lock. That way you have ensured silence
>> on the bus for the required time before closing the gate. You would still need
>> to bypass regmap, but only in this one place (but maybe you should bypass
>> regmap for opening the gate too, for symmetry).
> 
> To further detail the problem, the system is vulnerable from before the last write
> performed by sii902x_i2c_bypass_select to after we confirm we need the switch
> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time
> we could keep the parent adapter locked for, I guess I am stuck with a parent-locking
> architecture, aren't I?

If that problem description is correct, then yes, I think the *only* solution
is to combine the three parts "open bypass mode", "edid xfer" and "close bypass
mode", and to keep the i2c adapter locked during the procedure so that other
xfers do not creep in and crap thing up from the side. And one way to combine
the three parts is to use a parent-locked i2c gate. And since you need to keep
the i2c adapter locked over the whole procedure, you need to use unlocked xfers
(as you have already discovered). But how do you know that this problem
description is accurate? Why is it not ok for unrelated xfers to creep in
between opening the bypass mode and the edid xfer, and how do you know that
this is not ok?

> But I guess I could release it when it's not actually needed,

How would you figure out when it's not needed?

> or is this going to be a pain to maintain? Shall I just keep going with the parent-locking
> but using bare i2c transactions only within select and deselect and leave regmap
> to deal with everything else?

That's a possibility. Take care to not mess up any cached state in regmap though.
The registers you touch from select/deselect should probably be volatile or
something like that?

Cheers,
Peter