On Thu, 26 Oct 2023, Emil Abildgaard Svendsen <EMAS@xxxxxxxxxxxxxxx> wrote: > Currently reading EDID only works because usually only two EDID blocks > of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID > blocks. And the first EDID segment read works fine but E-EDID specifies > up to 128 segments. > > The logic is broken so change EDID segment index to multiple of 256 > bytes and not 128 (block size). > > Also change check of DDC status. Instead of silently not reading EDID > when in "IDLE" state [1]. Check if the DDC controller is in reset > instead (no HPD). "Also" in a commit message is often a good indication that the patch should be split to do the "also" in a separate patch. One "thing" per patch. Here, it'll be useful for debugging [1], too, to figure out which part broken things. (I suspect it's the status handling.) BR, Jani. [1] https://lore.kernel.org/r/5aa375f1-07cd-4e28-8cd5-7e10b4b05c6a@xxxxxxxxxx > > [1] > ADV7511 Programming Guide: Table 11: DDCController Status: > > 0xC8 [3:0] DDC Controller State > > 0000 In Reset (No Hot Plug Detected) > 0001 Reading EDID > 0010 IDLE (Waiting for HDCP Requested) > 0011 Initializing HDCP > 0100 HDCP Enabled > 0101 Initializing HDCP Repeater > > Signed-off-by: Emil Svendsen <emas@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 24 ++++++++++++-------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 8be235144f6d..c641c2ccd412 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -537,6 +537,8 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block, > size_t len) > { > struct adv7511 *adv7511 = data; > + struct device* dev = &adv7511->i2c_edid->dev; > + int edid_segment = block / 2; > struct i2c_msg xfer[2]; > uint8_t offset; > unsigned int i; > @@ -545,7 +547,7 @@ 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) { > + if (adv7511->current_edid_segment != edid_segment) { > unsigned int status; > > ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS, > @@ -553,15 +555,19 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block, > 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 (!(status & 0x0F)) { > + dev_dbg(dev, "DDC in reset no hot plug detected %x\n", > + status); > + return -EIO; > } > > + adv7511->edid_read = false; > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, > + edid_segment); > + ret = adv7511_wait_for_edid(adv7511, 200); > + if (ret < 0) > + return ret; > + > /* Break this apart, hopefully more I2C controllers will > * support 64 byte transfers than 256 byte transfers > */ > @@ -589,7 +595,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 = edid_segment; > } > > if (block % 2 == 0) -- Jani Nikula, Intel
