On Fri, Oct 27, 2023 at 12:51:07PM +0300, Jani Nikula wrote: > 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.) Thank you for the tip! I have now split it into two [1]. Yes, I believe you're correct, it's the status handling. [1] https://lore.kernel.org/all/20231027122214.599067-1-emas@xxxxxxxxxxxxxxx/ > > > 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
