[PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures

[Dmitry Baryshkov]

On Mon, May 20, 2024 at 08:16:14PM -0500, Adam Ford wrote:
> In the process of adding support for shared IRQ pins, a scenario
> was accidentally created where adv7511_irq_process returned
> prematurely causing the EDID to fail randomly.
> 
> Since the interrupt handler is broken up into two main helper functions,
> update both of them to treat the helper functions as IRQ handlers. These
> IRQ routines process their respective tasks as before, but if they
> determine that actual work was done, mark the respective IRQ status
> accordingly, and delay the check until everything has been processed.
> 
> This should guarantee the helper functions don't return prematurely
> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
> 
> Reported by: Liu Ying <victor.liu at nxp.com>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <aford173 at gmail.com>
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index ea271f62b214..ec0b7f3d889c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -401,7 +401,7 @@ struct adv7511 {
>  
>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>  int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>  #else
>  static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>  {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 44451a9658a3..4efb2cabf1b5 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>  	cec_received_msg(adv7511->cec_adap, &msg);
>  }
>  
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  {
>  	unsigned int offset = adv7511->info->reg_cec_offset;
>  	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  				ADV7511_INT1_CEC_RX_READY3;
>  	unsigned int rx_status;
>  	int rx_order[3] = { -1, -1, -1 };
> -	int i;
> +	int i, ret = 0;
> +	int irq_status = IRQ_NONE;
>  
> -	if (irq1 & irq_tx_mask)
> +	if (irq1 & irq_tx_mask) {
>  		adv_cec_tx_raw_status(adv7511, irq1);
> +		irq_status = IRQ_HANDLED;
> +	}
>  
>  	if (!(irq1 & irq_rx_mask))
> -		return;
> +		return irq_status;
>  
> -	if (regmap_read(adv7511->regmap_cec,
> -			ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
> -		return;
> +	ret = regmap_read(adv7511->regmap_cec,
> +			ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
> +	if (ret < 0)
> +		return ret;

Ok, maybe I was wrong with my previous suggestion. The code starts to
look more and more clumsy.  Do we really care about error status at all?
Maybe it's enough to return IRQ_NONE here from the IRQ handlers?

>  
>  	/*
>  	 * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  
>  		adv7511_cec_rx(adv7511, rx_buf);
>  	}
> +
> +	return IRQ_HANDLED;
>  }
>  
>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 66ccb61e2a66..56dd2d5a0376 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>  {
>  	unsigned int irq0, irq1;
>  	int ret;
> +	int cec_status;

cec_status ends up being unset if CEC is disabled.

> +	int irq_status = IRQ_NONE;
>  
>  	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
>  	if (ret < 0)
> @@ -478,38 +480,41 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* If there is no IRQ to handle, exit indicating no IRQ data */
> -	if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> -	    !(irq1 & ADV7511_INT1_DDC_ERROR))
> -		return -ENODATA;
> -
>  	regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
>  	regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>  
> -	if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
> +	if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
>  		schedule_work(&adv7511->hpd_work);
> +		irq_status = IRQ_HANDLED;
> +	}
>  
>  	if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
>  		adv7511->edid_read = true;
>  
>  		if (adv7511->i2c_main->irq)
>  			wake_up_all(&adv7511->wq);
> +		irq_status = IRQ_HANDLED;
>  	}
>  
>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> -	adv7511_cec_irq_process(adv7511, irq1);
> +	cec_status = adv7511_cec_irq_process(adv7511, irq1);
> +
> +	if (cec_status < 0)
> +		return cec_status;
>  #endif
>  
> -	return 0;
> +	/* If there is no IRQ to handle, exit indicating no IRQ data */
> +	if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
> +		return IRQ_HANDLED;
> +
> +	return IRQ_NONE;
>  }
>  
>  static irqreturn_t adv7511_irq_handler(int irq, void *devid)
>  {
>  	struct adv7511 *adv7511 = devid;
> -	int ret;
>  
> -	ret = adv7511_irq_process(adv7511, true);
> -	return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> +	return adv7511_irq_process(adv7511, true);

This should be return ret < 0 ? IRQ_NONE : ret. We should not be
returning negative error via irqreturn_t.

>  }
>  
>  /* -----------------------------------------------------------------------------
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry