On 26/03/2021 02:00, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Wed, Mar 24, 2021 at 09:53:32AM +0100, Hans Verkuil wrote:
>> While testing support for large (> 256 bytes) EDIDs on the Renesas
>> Koelsch board I noticed that the adv7511 bridge driver only read the
>> first two blocks.
>>
>> The media V4L2 version for the adv7511 (drivers/media/i2c/adv7511-v4l2.c)
>> handled this correctly.
>>
>> Besides a simple bug when setting the segment register (it was set to the
>> block number instead of block / 2), the logic of the code was also weird.
>> In particular reading the DDC_STATUS is odd: this is unrelated to EDID
>> reading.
>
> Bits 3:0 of DDC_STATUS report the DDC controller state, which can be
> used to wait until the DDC controller is idle (it reports, among other
> possible states, if an EDID read is in progress). Other options are
> possible of course, including waiting for ADV7511_INT0_EDID_READY as
> done in adv7511_wait_for_edid(), but I wonder if the !irq case in
> adv7511_wait_for_edid() wouldn't be better of busy-looping on the DDC
> status instead of running the interrupt handler manually. That's
> unrelated to this patch though.
The DDC status tests for other things as well, including HDCP.
I think it is pure luck that this code even worked:
if (adv7511->current_edid_segment != block / 2) {
unsigned int status;
ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
&status);
if (ret < 0)
return ret;
if (status != 2) {
adv7511->edid_read = false;
regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
block);
ret = adv7511_wait_for_edid(adv7511, 200);
if (ret < 0)
return ret;
}
What happens on power on is that the adv7511 starts reading the EDID.
So the DDC_STATUS is 1 (Reading EDID). This code is called, it falls
in the status != 2 block, it writes the EDID_SEGMENT with 0 (it already
is 0 after a power on), then waits for the EDID read to finish.
The only reason this works is that this code is called fast enough
after the device is powered on that it is still reading the EDID.
It fails if you want to read the next segment, since in that case the
status is 2 (IDLE) and it will never write the new segment to the
EDID_SEGMENT register.
And besides, status wasn't ANDed with 0xf either, and HDCP might
also be ongoing (should that be enabled in the future).
Regards,
Hans
