[PATCH] drm: bridge: adv7511: fix reading edid segments
Jani Nikula
jani.nikula at linux.intel.com
Fri Oct 27 09:51:07 UTC 2023
On Thu, 26 Oct 2023, Emil Abildgaard Svendsen <EMAS at bang-olufsen.dk> 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@kontron.de
>
> [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 at bang-olufsen.dk>
> ---
> 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
More information about the dri-devel
mailing list