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

From: "Harlozinski, Pawel" <pawel.harlozinski@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: cezary.rojewski@intel.com, patch@alsa-project.org,
	alsa-devel@alsa-project.org, lgirdwood@gmail.com,
	pierre-louis.bossart@linux.intel.com, broonie@kernel.org
Subject: Re: [alsa-devel] [RFC PATCH 1/3] ALSA:hda: Simplify and clear calculating SDxFMT
Date: Fri, 6 Sep 2019 15:41:15 +0200	[thread overview]
Message-ID: <f2ce89c3-f28f-091a-70a0-01da18ce25f3@linux.intel.com> (raw)
In-Reply-To: <s5hblvzguqb.wl-tiwai@suse.de>

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.

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 
supportedformats for stream DMA.

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@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@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel