[alsa-devel] [RFC PATCH 1/3] ALSA:hda: Simplify and clear calculating SDxFMT

[Takashi Iwai]

On Fri, 06 Sep 2019 15:41:15 +0200,
Harlozinski, Pawel wrote:
> 
> Thanks for noticing !
> 
> Indeed  snd_hdac_query_supported_pcm() also should be aligned. 
> Could you help a bit there with explaining background of current
> implementation ?
> I'm wondering  why SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE  is set instead of 
> SNDRV_PCM_FMTBIT_S32_LE in case of using 32 bits and AC_WCAP_DIGITAL.

That's some heuristic; if the device requires really 32bit, it's most
likely not real 32bit but with the IEC status bits.  This never has
been a problem, so there is no reason to change it.  Of course, if you
have a real device that requires different format, then we can fix
that.


> Whats more, bps looks like redundant information there, as pcm formats have
> width defined in sound/core/pcm-misc.c.
> 
> I would leave 20 and 24 bits per sample there, as those are supported formats
> for stream DMA.

The HD-audio spec 20 and 24 bits are MSB-aligned, AFAIK, so they are
not what you supposed with S20_LE or S24_LE format.

Again, if you have really some device that behaves differently, it's a
different story.

In general, this kind of "cleanup" would easily lead to some breakage,
and we have to be really careful.


thanks,

Takashi

> 
> Regards,
> Paweł
> 
> On 9/5/2019 7:47 AM, Takashi Iwai wrote:
> 
>     On Thu, 05 Sep 2019 07:32:59 +0200,
>     Pawel Harlozinski wrote:
>     
>         Set SDxFMT based only on given format, due to maxbps not always being set.
>         Split cases for formats 20,24,32 bits.
>         For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so it will end up
>         with same mask.
>         
>     This function corresponds to snd_hdac_query_supported_pcm(), so this
>     patch breaks.
>     
>     Basically the cases of 20 and 24 are superfluous and can be dropped.
>     It's there just to be sure.
> 
>     Takashi
> 
>         Signed-off-by: Pawel Harlozinski <pawel.harlozinski at linux.intel.com>
>         ---
>          sound/hda/hdac_device.c | 11 +++++------
>          1 file changed, 5 insertions(+), 6 deletions(-)
>         
>         diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
>         index b26cc93e7e10..add758e11b85 100644
>         --- a/sound/hda/hdac_device.c
>         +++ b/sound/hda/hdac_device.c
>         @@ -759,14 +759,13 @@ unsigned int snd_hdac_calc_stream_format(unsigned int rate,
>                         val |= AC_FMT_BITS_16;
>                         break;
>                 case 20:
>         +               val |= AC_FMT_BITS_20;
>         +               break;
>                 case 24:
>         +               val |= AC_FMT_BITS_24;
>         +               break;
>                 case 32:
>         -               if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)
>         -                       val |= AC_FMT_BITS_32;
>         -               else if (maxbps >= 24)
>         -                       val |= AC_FMT_BITS_24;
>         -               else
>         -                       val |= AC_FMT_BITS_20;
>         +               val |= AC_FMT_BITS_32;
>                         break;
>                 default:
>                         return 0;
>         -- 
>         2.17.1
> 
>     _______________________________________________
>     Alsa-devel mailing list
>     Alsa-devel at alsa-project.org
>     https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
>