Re: [PATCH] drm/bridge: anx7625: Use irq flags from devicetree

[Nícolas F. R. A. Prado]

On Fri, Apr 08, 2022 at 12:15:44PM +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 03:33, Nícolas F. R. A. Prado ha scritto:
> > Read the irq flags, like which edge to trigger on, from the devicetree
> > and use those when registering the irq instead of hardcoding them.
> > In case none was specified, fallback to falling edge trigger.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > 
> > ---
> > 
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 6516f9570b86..97d954b8cc12 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -2588,6 +2588,7 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >   	struct anx7625_platform_data *pdata;
> >   	int ret = 0;
> >   	struct device *dev = &client->dev;
> > +	unsigned long irqflags;
> >   	if (!i2c_check_functionality(client->adapter,
> >   				     I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > @@ -2639,10 +2640,13 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >   			goto free_hdcp_wq;
> >   		}
> > +		irqflags = irq_get_trigger_type(client->irq);
> 
> Nicolas, this is not necessary... what are you trying to solve?

Hi Angelo,

the ANX7625 DP bridge for the mt8192-asurada-spherion machine specifies
IRQ_TYPE_LEVEL_LOW for its interrupt trigger. The current code would just ignore
that setting and always configure it as falling edge instead. This patch solves
that, which doesn't have any clear behavior impact as a falling edge always
precedes the low level anyway, but from 4c83c071b784 ("Input: elants_i2c - do
not clobber interrupt trigger on x86"), which is similar to this commit for a
different driver, mentions that triggering on low level instead should be more
stable.

In any case, this commit is really only about honoring the irq trigger setting
in DT when present.

> 
> > +		if (!irqflags)
> > +			irqflags = IRQF_TRIGGER_FALLING;
> 
> ...and this is already done for you by __setup_irq(), check kernel/irq/manage.c.

Not that I could see...

I think what you're thinking of is, in __setup_irq():

	/*
	 * If the trigger type is not specified by the caller,
	 * then use the default for this interrupt.
	 */
	if (!(new->flags & IRQF_TRIGGER_MASK))
		new->flags |= irqd_get_trigger_type(&desc->irq_data);

As the comment says, this only sets the trigger when it isn't already specified.
Besides, it would try to get it from DT which has already failed in the first
place.

> 
> > +
> >   		ret = devm_request_threaded_irq(dev, platform->pdata.intp_irq,
> >   						NULL, anx7625_intr_hpd_isr,
> > -						IRQF_TRIGGER_FALLING |
> > -						IRQF_ONESHOT,
> > +						irqflags | IRQF_ONESHOT,
> >   						"anx7625-intp", platform);

I guess another option would be to make this whole patch simply omit the trigger
setting here:

-						IRQF_TRIGGER_FALLING |
-						IRQF_ONESHOT,
+						IRQF_ONESHOT,

so that __setup_irq() would already try to fetch it from the DT. But then 
not having the setting present in the DT wouldn't fallback to the falling edge
trigger, instead it would be set to IRQ_TYPE_NONE. That would break the ABI, so
it's a no-go.

> Partial devm_request_threaded_irq() flow, to make you understand what's going on:
> 
> devm_request_threaded_irq() -> request_threaded_irq() -> __setup_irq()
> 
> 
> That said, congrats for solving that external display issue, but I'm sure that this
> commit is not part of any solution and, if it is, you should recheck your machine's
> DT, as the mistake must be there, then!

Thanks! This commit is not part of the solution for the external display issue
indeed.

Thanks,
Nícolas