linux-kernel - Re: [RESEND RFC PATCH 3/3] ASoC: mediatek: Add AFE platform driver

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20150418175139.GG26185@sirena.org.uk>
Date:	Sat, 18 Apr 2015 18:51:39 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Koro Chen <koro.chen@...iatek.com>
Cc:	robh+dt@...nel.org, matthias.bgg@...il.com, perex@...ex.cz,
	tiwai@...e.de, srv_heupstream@...iatek.com,
	linux-mediatek@...ts.infradead.org, s.hauer@...gutronix.de,
	galak@...eaurora.org, lgirdwood@...il.com,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [RESEND RFC PATCH 3/3] ASoC: mediatek: Add AFE platform driver

On Fri, Apr 10, 2015 at 04:14:09PM +0800, Koro Chen wrote:

> +	if (memif->use_sram) {
> +		struct snd_dma_buffer *dma_buf = &substream->dma_buffer;
> +		int size = params_buffer_bytes(params);
> +
> +		memif->buffer_size = size;
> +		memif->phys_buf_addr = afe->sram_phy_address;
> +
> +		dma_buf->bytes = size;
> +		dma_buf->area = (unsigned char *)afe->sram_address;
> +		dma_buf->addr = afe->sram_phy_address;
> +		dma_buf->dev.type = SNDRV_DMA_TYPE_DEV;
> +		dma_buf->dev.dev = substream->pcm->card->dev;
> +		snd_pcm_set_runtime_buffer(substream, dma_buf);
> +	} else {
> +		ret = snd_pcm_lib_malloc_pages(substream,
> +					       params_buffer_bytes(params));
> +		if (ret < 0)
> +			return ret;
> +
> +		memif->phys_buf_addr = substream->runtime->dma_addr;
> +		memif->buffer_size = substream->runtime->dma_bytes;
> +	}

Ah, so the SRAM is directly memory mappable.  Nice.  But we have a
limited amount of it so need to allocate it to a device somehow based on
some factor I guess?

> +static int mtk_afe_set_adda_dac_out(struct mtk_afe *afe, uint32_t rate)
> +{
> +	u32 audio_i2s_dac = 0;
> +	u32 con0, con1;
> +
> +	/* set dl src2 */
> +	con0 = (mtk_afe_adda_fs(rate) << 28) | (0x03 << 24) | (0x03 << 11);
> +
> +	/* 8k or 16k voice mode */
> +	if (con0 == 0 || con0 == 3)
> +		con0 |= 0x01 << 5;

This all looks a bit magic, some defines would not go amiss here.

> +	/* SA suggests to apply -0.3db to audio/speech path */
> +	con0 = con0 | (0x01 << 1);
> +	con1 = 0xf74f0000;

More magic numbers!  This also suggests that there is a volume control
lurking in here which could usefully be exposed to applications?

> +static void mtk_afe_pmic_shutdown(struct mtk_afe *afe,
> +				  struct snd_pcm_substream *substream)
> +{
> +	/* output */
> +	regmap_update_bits(afe->regmap, AFE_ADDA_DL_SRC2_CON0, 1, 0);
> +	regmap_update_bits(afe->regmap, AFE_I2S_CON1, 1, 0);
> +
> +	/* input */
> +	regmap_update_bits(afe->regmap, AFE_ADDA_UL_SRC_CON0, 1, 0);
> +	/* disable ADDA */
> +	regmap_update_bits(afe->regmap, AFE_ADDA_UL_DL_CON0, 1, false);
> +}

This is looking like exposing the routing and using DAPM might save a
bunch of code?  Overall my main thought looking at the code here and
what the hardware was described as doing is that it'd all be simpler if
it were a DPCM based thing using DAPM for power.  I think I'd like to
see a strong reason for not using at least DPCM.

> +		if (rate == MTK_AFE_I2S_RATE_8K)
> +			voice_mode = 0;
> +		else if (rate == MTK_AFE_I2S_RATE_16K)
> +			voice_mode = 1;
> +		else if (rate == MTK_AFE_I2S_RATE_32K)
> +			voice_mode = 2;
> +		else if (rate == MTK_AFE_I2S_RATE_48K)
> +			voice_mode = 3;

This should be a switch statement.

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