Re: [PATCH] drm/bridge: adv7511: Clear HPD IRQ before powering on device during resume()

[Laurent Pinchart]

On Mon, Mar 16, 2026 at 05:59:57PM +0000, Biju Das wrote:
> On 16 March 2026 14:03, Laurent Pinchart wrote:
> > On Fri, Dec 19, 2025 at 10:46:53AM +0000, Biju wrote:
> > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >
> > > On RZ/G3E SMARC EVK using PSCI, s2ram powers down the SoC. Testing
> > > ADV7535 IRQ configured as edge-triggered interrupt on RZ/G3E SMARC EVK
> > > shows that it is missing HPD IRQ during system resume, as the status
> > > change occurs before the IRQ/pincontrol resume. Once the status bit is
> > > set, there won't be any further IRQ unless the status bit is cleared.
> > >
> > > Clear any pending HPD IRQs before powering on the ADV7535 device to
> > > deliver HPD interrupts after resume().
> > 
> > This issue doesn't seem to be specific to the ADV7511. Any device that uses an edge-triggered
> > interrupt could suffer from the same problem.
> > Implementing a work around in the driver doesn't seem to be a solution that would scale.
> 
> I don't see any bridge device is complaining about similar issues in Linux kernel.

This is exactly why this patch concerns me. The issue doesn't seem to be
specific to the ADV7511, yet no other bridge driver implements anything
similar. It seems to indicate something else is wrong.

I understand and agree with the analysis of the issue (although I find
it weird that the interrupt controller or pin controller would be
resumed after the ADV7511), but any device with an edge-triggered
interrupt should then suffer from the same problem. This means it
shouldn't be handled at individual drivers level, unless there's
something I'm missing that makes the problem very specific to the
ADV7511. Otherwise, a more generic solution is needed.

> Looks like, other bridge devices do not have such restriction.
>
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 +
> > >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 32 ++++++++++++++++++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > > index 8be7266fd4f4..03aa23836ca4 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > > @@ -393,6 +393,7 @@ struct adv7511 {
> > >  	bool cec_enabled_adap;
> > >  	struct clk *cec_clk;
> > >  	u32 cec_clk_freq;
> > > +	bool suspended;
> > >  };
> > >
> > >  static inline struct adv7511 *bridge_to_adv7511(struct drm_bridge
> > > *bridge) diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index b9be86541307..8d9467187d7c 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -790,6 +790,25 @@ static void adv7511_bridge_atomic_enable(struct drm_bridge *bridge,
> > >  	struct drm_connector_state *conn_state;
> > >  	struct drm_crtc_state *crtc_state;
> > >
> > > +	if (adv->i2c_main->irq && adv->suspended) {
> > > +		unsigned int irq;
> > > +
> > > +		/*
> > > +		 * If ADV7511 IRQ is configured as edge triggered interrupt, it
> > > +		 * will miss the IRQ during system resume as the status change
> > > +		 * occurs before IRQ/pincontrol resume. Once the status bit is
> > 
> > That seems very platform-specific.
> 
> OK, I will reword something like. Is it ok for you?
> 
> The HPD status change occurs before the interrupt/pin control resume.
> Once the status bit is set, there will be no further delivery of interrupts
> unless the status bit is cleared. Therefore, clear the interrupt status bit
> for further delivery of HPD interrupts.
> 
> > 
> > > +		 * set there won't be any further IRQ unless the status bit is
> > > +		 * cleared. So, clear the IRQ status bit for further delivery
> > > +		 * of HPD IRQ.
> > > +		 */
> > > +		regmap_read(adv->regmap, ADV7511_REG_INT(0), &irq);
> > > +		if (irq & ADV7511_INT0_HPD)
> > > +			regmap_write(adv->regmap, ADV7511_REG_INT(0),
> > > +				     ADV7511_INT0_HPD);
> > 
> > Why do you need to read and test the IRQ here ? If ADV7511_INT0_HPD isn't set, a write will be a no-op
> > and will keep it cleared. If it is set, it will clear it. It seems that an unconditional
> > 
> > 		regmap_write(adv->regmap, ADV7511_REG_INT(0),
> > 			     ADV7511_INT0_HPD);
> > 
> > should be enough.
> 
> Agreed, I will make this unconditional.

-- 
Regards,

Laurent Pinchart