linux-kernel - Re: [PATCH v4 13/15] ASoC: codec/dw-hdmi-audio: add codec driver for dw hdmi audio

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: <20150326181106.GY3572@sirena.org.uk>
Date:	Thu, 26 Mar 2015 11:11:06 -0700
From:	Mark Brown <broonie@...nel.org>
To:	Yakir Yang <ykk@...k-chips.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>, djkurtz@...omium.org,
	dianders@...omium.org, linux-rockchip@...ts.infradead.org,
	David Airlie <airlied@...ux.ie>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Andy Yan <andy.yan@...k-chips.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Fabio Estevam <fabio.estevam@...escale.com>,
	dri-devel@...ts.freedesktop.org, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Brian Austin <brian.austin@...rus.com>,
	Bard Liao <bardliao@...ltek.com>,
	Oder Chiou <oder_chiou@...ltek.com>,
	Max Filippov <jcmvbkbc@...il.com>,
	Axel Lin <axel.lin@...ics.com>, Arnd Bergmann <arnd@...db.de>,
	Jyri Sarha <jsarha@...com>, Sean Cross <xobs@...agi.com>,
	Ben Zhang <benzh@...omium.org>, linux-kernel@...r.kernel.org,
	alsa-devel@...a-project.org, mmind00@...glemail.com,
	marcheu@...omium.org, mark.yao@...k-chips.com
Subject: Re: [PATCH v4 13/15] ASoC: codec/dw-hdmi-audio: add codec driver for
 dw hdmi audio

On Sat, Feb 28, 2015 at 09:59:20PM -0500, Yakir Yang wrote:

> codec driver creat an standard alsa device, than config audio
> and report jack status through some callback interfaces that
> dw_hdmi driver support.

Looking at this it's not althogether clear to me how specific this is to
the Designware hardware - it looks like it's all callbacks into the main
driver doing pretty generic things apart from the fact that we request
an interrupt here (but then use it to do another callback into the
driver).

Please also try to only CC relevant people on mails - you've got a
*very* large list of people there and for a lot of them it's hard to
understand why you've copied them.  Copying people adds to the amount of
mail they need to read so it's good to try to stay relevant.

> +	if (jack_status != hdmi->jack_status) {
> +		snd_soc_jack_report(&hdmi->jack, jack_status,
> +				    SND_JACK_LINEOUT);

We may need a new jack type here, or perhaps we ought to just be
reporting the jack status via extcon?

> +		hdmi->jack_status = jack_status;
> +
> +		dev_info(hdmi->dev, "jack report [%d]\n", hdmi->jack_status);

Please remove this and all the other prints, it's far too noisy.

> +/* we don't want this irq mark with IRQF_ONESHOT flags,
> + * so we build an irq_default_primary_handler here */
> +static irqreturn_t snd_dw_hdmi_hardirq(int irq, void *dev_id)
> +{
> +	return IRQ_WAKE_THREAD;
> +}

Why do we not want to use IRQF_ONESHOT?

> +static int dw_hdmi_audio_remove(struct platform_device *pdev)
> +{
> +	struct snd_dw_hdmi *hdmi = platform_get_drvdata(pdev);
> +
> +	snd_soc_unregister_codec(&pdev->dev);
> +	devm_free_irq(&pdev->dev, hdmi->data.irq, hdmi);
> +	devm_kfree(&pdev->dev, hdmi);

Explicitly freeing devm_ things seems to be missing the point a bit...

> +static const struct of_device_id dw_hdmi_audio_ids[] = {
> +	{ .compatible = DRV_NAME, },
> +	{ }
> +};

Your driver name didn't have a vendor prefix, this is broken - you
should probably just remove DRV_NAME and use the string directly in the
few places it's used.  It's also not clear to me that this is a separate
device from the parent device and should therefore appear separately in
DT at all.

> +static struct platform_driver dw_hdmi_audio_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,

No need to assign owner any more.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)