[PATCHv2] drm/bridge: adv7511: fix support for large EDIDs

Commit Message

Hans Verkuil March 26, 2021, 1:19 p.m. UTC

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.

The reworked code just waits for any EDID segment reads to finish (this
does nothing if the a segment is already read), checks if the desired
segment matches the read segment, and if not, then it requests the new
segment and waits again for the EDID segment to be read.

Finally it checks if the currently buffered EDID segment contains the
desired EDID block, and if not it will update the EDID buffer from
the adv7511.

Tested with my Koelsch board and with EDIDs of 1, 2, 3 and 4 blocks.

Signed-off-by: Hans Verkuil <[e-mail]>
Tested-by: Niklas Söderlund <[e-mail]>
---
Changes since v2: make current_edid_segment an int (it's set to -1 after all)
and use that instead of reading ADV7511_REG_EDID_SEGMENT. Also sprinkle a
few comments in the code.
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +-
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 40 +++++++++++++-------
 2 files changed, 27 insertions(+), 15 deletions(-)

Patch

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index a9bb734366ae..3dd29c578fc9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -342,7 +342,7 @@  struct adv7511 {
 	unsigned int f_audio;
 	unsigned int audio_source;

-	unsigned int current_edid_segment;
+	int current_edid_segment;
 	uint8_t edid_buf[256];
 	bool edid_read;

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 76555ae64e9c..43fefdd8d92b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -327,7 +327,12 @@  static void adv7511_set_link_config(struct adv7511 *adv7511,

 static void __adv7511_power_on(struct adv7511 *adv7511)
 {
+	/*
+	 * The adv7511 will start reading the first EDID segment as
+	 * soon as it is powered on.
+	 */
 	adv7511->current_edid_segment = -1;
+	adv7511->edid_read = false;

 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN, 0);
@@ -526,6 +531,7 @@  static int adv7511_wait_for_edid(struct adv7511 *adv7511, int timeout)
 static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 				  size_t len)
 {
+	unsigned int need_segment = block / 2;
 	struct adv7511 *adv7511 = data;
 	struct i2c_msg xfer[2];
 	uint8_t offset;
@@ -535,23 +541,29 @@  static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 	if (len > 128)
 		return -EINVAL;

-	if (adv7511->current_edid_segment != block / 2) {
-		unsigned int status;
+	/* wait for any ongoing EDID segment reads to finish */
+	adv7511_wait_for_edid(adv7511, 200);

-		ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
-				  &status);
+	/*
+	 * If the current read segment does not match what we need, then
+	 * write the new segment and wait for it to be read.
+	 *
+	 * Note that after power on the adv7511 starts reading segment 0
+	 * of the EDID automatically. So if current_edid_segment < 0, then
+	 * we do not need to write the EDID_SEGMENT register again, since
+	 * it is already reading segment 0.
+	 */
+	if (adv7511->current_edid_segment >= 0 &&
+	    adv7511->current_edid_segment != need_segment) {
+		adv7511->edid_read = false;
+		regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
+			     need_segment);
+		ret = adv7511_wait_for_edid(adv7511, 200);
 		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;
-		}
-
+	if (adv7511->current_edid_segment != need_segment) {
 		/* Break this apart, hopefully more I2C controllers will
 		 * support 64 byte transfers than 256 byte transfers
 		 */
@@ -579,7 +591,7 @@  static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 			offset += 64;
 		}

-		adv7511->current_edid_segment = block / 2;
+		adv7511->current_edid_segment = need_segment;
 	}

 	if (block % 2 == 0)