linux-kernel - Re: [alsa-devel] [PATCH v2 2/9] ASoC: mediatek: implement mediatek common structure

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]

Date:	Tue, 7 Jun 2016 16:54:27 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Garlic Tseng <garlic.tseng@...iatek.com>
Cc:	tiwai@...e.de, srv_heupstream@...iatek.com,
	linux-mediatek@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	alsa-devel@...a-project.org, koro.chen@...iatek.com,
	PC.Liao@...iatek.com, ir.lian@...iatek.com
Subject: Re: [alsa-devel] [PATCH v2 2/9] ASoC: mediatek: implement mediatek
 common structure

On Fri, Jun 03, 2016 at 12:56:17PM +0800, Garlic Tseng wrote:

> +	/*enable agent*/

Lots of comments with missing spaces in them, there's quite a few
examples of that in here.

> +EXPORT_SYMBOL(mtk_afe_fe_startup);

All the ASoC and regmap APIs are _GPL(), you really shouldn't export new
interfaces based on them without it - the point with the _GPL() is that
non-GPL code shouldn't be able to use the APIs.

> +EXPORT_SYMBOL(mtk_afe_fe_hw_params);

Do you need to export the individual ops rather than just the ops
structure?

> +int mtk_afe_fe_trigger(struct snd_pcm_substream *substream, int cmd,
> +		       struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_pcm_runtime * const runtime = substream->runtime;
> +	struct mtk_base_afe *afe = snd_soc_platform_get_drvdata(rtd->platform);
> +	struct mtk_base_afe_memif *memif = &afe->memif[rtd->cpu_dai->id];
> +	struct mtk_base_afe_irq *irqs = &afe->irqs[memif->irq_usage];
> +	const struct mtk_base_irq_data *irq_data = irqs->irq_data;
> +	unsigned int counter = runtime->period_size;
> +	int fs;
> +
> +	dev_info(afe->dev, "%s %s cmd=%d\n", __func__, memif->data->name, cmd);

That's way too noisy, dev_dbg() at most.

> +static DEFINE_MUTEX(irqs_lock);
> +int mtk_dynamic_irq_acquire(struct mtk_base_afe *afe)
> +{
> +	int i;
> +
> +	mutex_lock(&irqs_lock);
> +	for (i = 0; i < afe->irqs_size; ++i) {

Why is the lock global and not part of the AFE struct?

> +void mtk_simple_isr(struct mtk_base_afe *afe, struct mtk_base_afe_memif *memif)
> +{
> +	snd_pcm_period_elapsed(memif->substream);
> +}
> +EXPORT_SYMBOL(mtk_simple_isr);

Is this really worth it over just calling _period_elapsed() directly?

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