From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 3/6] ASoC: Intel: Add Haswell and Broadwell PCM platform driver Date: Fri, 21 Feb 2014 08:11:12 +0100 Message-ID: References: <1392932927-9725-1-git-send-email-liam.r.girdwood@linux.intel.com> <1392932927-9725-3-git-send-email-liam.r.girdwood@linux.intel.com> <20140221051603.GC25940@sirena.org.uk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 3CF6526179C for ; Fri, 21 Feb 2014 08:11:13 +0100 (CET) In-Reply-To: <20140221051603.GC25940@sirena.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Mark Brown Cc: Liam Girdwood , alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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