[alsa-devel] [PATCH v8 1/6] ALSA: pcm: add IEC958 channel status helper for hw_params

[Takashi Iwai]

On Tue, 29 Mar 2016 19:23:12 +0200,
Russell King - ARM Linux wrote:
> 
> On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote:
> > On Thu, 17 Mar 2016 13:22:29 +0100,
> > Jyri Sarha wrote:
> > > 
> > > Add IEC958 channel status helper that gets the audio properties from
> > > snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to
> > > produce the channel status bits already in audio stream configuration
> > > phase.
> > > 
> > > Signed-off-by: Jyri Sarha <jsarha at ti.com>
> > 
> > This patch looks almost good to me, but...
> > 
> > > @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> > >  			     IEC958_AES4_CON_MAX_WORDLEN_24;
> > >  			break;
> > >  		case 24:
> > > +		case 32: /* Assume 24-bit width for 32-bit samples. */
> > >  			ws = IEC958_AES4_CON_WORDLEN_24_20 |
> > >  			     IEC958_AES4_CON_MAX_WORDLEN_24;
> > >  			break;
> > 
> > ... this change is silently slipped in.  It should be mentioned in the
> > changelog, or split to another patch, as this is basically an
> > orthogonal change.
> 
> Does it even make sense - AES doesn't have support for 32-bit samples,
> it can only ever truncate them down to 24-bit.

Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on
real devices, mostly on PCI ones.  So, adding the value 32 check there
itself is valid, I suppose.  It's rather due to a bad design in the
current API.

The actual bits should be specified hw_params.msbits field, instead.
But, not all drivers set this properly, unfortunately.


Takashi