linux-kernel - Re: [PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140122222705.GL15937@n2100.arm.linux.org.uk>
Date:	Wed, 22 Jan 2014 22:27:05 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Jean-Francois Moine <moinejf@...e.fr>
Cc:	dri-devel@...ts.freedesktop.org, Dave Airlie <airlied@...il.com>,
	Rob Clark <robdclark@...il.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 15/24] drm/i2c: tda998x: use irq for connection
	status and EDID read

On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
> This patch adds the optional treatment of the tda998x IRQ.
> 
> The interrupt function is used to know the display connection status
> without polling and to speedup reading the EDID.
> 
> The interrupt number may be defined either in the DT or at encoder set
> config time for non-DT boards.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@...e.fr>
> ---
> v3
>     - remarks from Russell King
> 	- move the setting of the wq_edid_wait flag before asking the
> 	  device to read the EDID
> 	  Thanks about this bug fix, but I don't see the other problem:
> 	  there is no reason for the read_edid_block function to be
> 	  called more than once...

It will happen whenever DRM asks the connector for the modes.  However,
since the read process is triggered each time, I think this is fine.

> 	- use '0' as 'no irq'
> 	- remove the useless delay after irq init

Some further comments...

> @@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>  		priv->audio_port = p->audio_cfg;
>  		priv->audio_format = p->audio_format;
>  	}
> +
> +	priv->irq = p->irq;
> +	if (p->irq)
> +		tda_irq_init(priv);

If we're going to do it this way, this should probably release the IRQ if
there was one before re-claiming it, just in case this function gets called
more than once by some driver using it.

The alternative is, as I said before, to use the infrastructure which is
already there, namely setting the interrupt via struct i2c_client's
irq member.  Yes, that doesn't satisfy Sebastian's comment about using
a GPIO, but there's no sign of GPIO usage in here at the moment anyway.
So we might as well use what's already provided.

That would then avoid the need to deal with IRQ changes etc in
tda998x_encoder_set_config().

In any case, I've tested this patch in both non-IRQ and IRQ modes, and it
appears to work - but I'd like the suggestion above before I provide any
attributations.

Thanks.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/