Hi Hans, Thanks for your patch. On 2021-03-24 09:53:32 +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. > > 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 <hverkuil-cisco@xxxxxxxxx> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > --- > Testing on the Renesas board also requires these two adv7604 patches > if you want to test with an HDMI cable between the HDMI input and output: > > https://patchwork.linuxtv.org/project/linux-media/patch/00882808-472a-d429-c565-a701da579ead@xxxxxxxxx/ > https://patchwork.linuxtv.org/project/linux-media/patch/c7093e76-ffb4-b19c-f576-b264f935a3ce@xxxxxxxxx/ > --- > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 76555ae64e9c..9e8db1c60167 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -328,6 +328,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, > static void __adv7511_power_on(struct adv7511 *adv7511) > { > adv7511->current_edid_segment = -1; > + adv7511->edid_read = false; > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, > ADV7511_POWER_POWER_DOWN, 0); > @@ -529,29 +530,35 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block, > struct adv7511 *adv7511 = data; > struct i2c_msg xfer[2]; > uint8_t offset; > + unsigned int cur_segment; > unsigned int i; > int ret; > > if (len > 128) > return -EINVAL; > > - if (adv7511->current_edid_segment != block / 2) { > - unsigned int status; > + /* wait for any EDID segment reads to finish */ > + adv7511_wait_for_edid(adv7511, 200); > > - ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS, > - &status); > + ret = regmap_read(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, &cur_segment); > + if (ret < 0) > + return ret; > + > + /* > + * If the current read segment does not match what we need, then > + * write the new segment and wait for it to be read. > + */ > + if (cur_segment != block / 2) { > + adv7511->edid_read = false; > + cur_segment = block / 2; > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, > + cur_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 != cur_segment) { > /* Break this apart, hopefully more I2C controllers will > * support 64 byte transfers than 256 byte transfers > */ > @@ -579,7 +586,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 = cur_segment; > } > > if (block % 2 == 0) -- Regards, Niklas Söderlund
