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

From: Takashi Iwai <tiwai@suse.de>
To: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>,
	alsa-devel@alsa-project.org
Subject: Re: [PATCH 3/6] ASoC: Intel: Add Haswell and Broadwell PCM platform driver
Date: Fri, 21 Feb 2014 08:11:12 +0100	[thread overview]
Message-ID: <s5htxbtvt2n.wl%tiwai@suse.de> (raw)
In-Reply-To: <20140221051603.GC25940@sirena.org.uk>

At Fri, 21 Feb 2014 14:16:03 +0900,
Mark Brown wrote:
> 
> 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.

A straightforward implementation would be just to expose this 31bit
raw value and declare it as a linear dB via TLV.  The proper dB
conversion is done in alsa-lib.

But, this depends on the use case; if an expected application rather
doesn't dare to think of dB value, it's another way to provide the
converted mixer values in the driver.  Some drivers do it, too.

BTW, you need no table for this volume value at all.  volume_map[] can
be replaced like
	#define volume_map(i)	((1U << (i) - 1)

> 
> > +	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.

This seems restored in hsw_pcm_open().

+static int hsw_pcm_open(struct snd_pcm_substream *substream)
+{
....
+	/* Set previous saved volume */
+	sst_hsw_stream_set_volume(hsw, pcm_data->stream, 0,
+			0, pcm_data->volume[0]);
+	sst_hsw_stream_set_volume(hsw, pcm_data->stream, 0,
+			1, pcm_data->volume[1]);

Takashi