Re: [PATCH 3/6] ASoC: Intel: Add Haswell and Broadwell PCM platform driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1908 bytes --]

On Thu, Feb 20, 2014 at 09:48:44PM +0000, Liam Girdwood wrote:

Applied, thanks.  Again a few comments below.

> +/* simple volume table */
> +static const u32 volume_map[] = {
> +	HSW_VOLUME_MAX >> 30,

Interesting one here.  On the one hand we always say to present the
hardware features as directly as possible to the application layer.  On
the other hand I'd not be surprised if some userspaces failed to deal
constructively with a full 31 bits of linearly mapped volume control
(but I'm not sure how common they are and really they ought to be
fixed).  Anyway, not a problem more just a comment.

> +	if (!pcm_data->stream) {
> +		pcm_data->volume[0] =
> +			hsw_mixer_to_ipc(ucontrol->value.integer.value[0]);
> +		pcm_data->volume[1] =
> +			hsw_mixer_to_ipc(ucontrol->value.integer.value[1]);
> +		mutex_unlock(&pcm_data->mutex);
> +		return 0;
> +	}

It looks like we only record the volume when the stream is idle.  What
happens if the user changes the volume while it's idle, starts playing,
changes again then stops playing?  It's possible I missed the bit where
it gets saved but I did look.

> +	if (!pcm_data->stream) {
> +		ucontrol->value.integer.value[0] =
> +			hsw_ipc_to_mixer(pcm_data->volume[0]);
> +		ucontrol->value.integer.value[1] =
> +			hsw_ipc_to_mixer(pcm_data->volume[1]);
> +		mutex_unlock(&pcm_data->mutex);
> +		return 0;
> +	}
> +
> +	sst_hsw_stream_get_volume(hsw, pcm_data->stream, 0, 0, &volume);
> +	ucontrol->value.integer.value[0] = hsw_ipc_to_mixer(volume);
> +	sst_hsw_stream_get_volume(hsw, pcm_data->stream, 0, 1, &volume);
> +	ucontrol->value.integer.value[1] = hsw_ipc_to_mixer(volume);
> +	mutex_unlock(&pcm_data->mutex);

Can the DSP adjust its own volume autonomously?

> +static int create_adsp_page_table(struct hsw_priv_data *pdata,
> +	struct snd_soc_pcm_runtime *rtd,
> +	unsigned char *dma_area, size_t size, int pcm, int stream)

No helper for this?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]