Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 26 Oct 2023, Emil Abildgaard Svendsen <EMAS@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@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