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

From: Mark Brown <broonie@kernel.org>
To: Takashi Iwai <tiwai@suse.de>
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: Sun, 23 Feb 2014 12:19:58 +0900	[thread overview]
Message-ID: <20140223031958.GX25940@sirena.org.uk> (raw)
In-Reply-To: <s5htxbtvt2n.wl%tiwai@suse.de>

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

On Fri, Feb 21, 2014 at 08:11:12AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

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

Yes, exactly.  This should work well with most userspaces, the only
thing that would worry me would be something basic that makes the user
manually step through thousands of tiny volume steps.  I don't think any
modern application layer should have an issue there though and I'm not
sure if I'd care about such application layers myself.

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

It does mean it's easier to update the chosen steps in future though.

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

Yes, it's restored there - what concerned me was that the variables
didn't seem to be being updated while the stream was running, it only
wrote to the hardware so if the volume had subsequently been changed on
a running stream then the older value from a closed stream would be
written out.  It'd at least be more obviously correct to just update the
variables no matter what.

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

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