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

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

[Pawel Harlozinski]

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.

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] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Pawel Harlozinski]

Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
with alsa library.

Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index bbe6eb1ff5d2..09d0a2a2dce8 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -119,6 +119,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_RATE_192000		(1<<12)		/* 192000Hz */
 #define SNDRV_PCM_RATE_352800		(1<<13)		/* 352800Hz */
 #define SNDRV_PCM_RATE_384000		(1<<14)		/* 384000Hz */
+#define SNDRV_PCM_RATE_24000		(1<<15)		/* 24000Hz */
 
 #define SNDRV_PCM_RATE_CONTINUOUS	(1<<30)		/* continuous range */
 #define SNDRV_PCM_RATE_KNOT		(1<<31)		/* supports more non-continuos rates */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 11e653c8aa0e..f52f28e3edb1 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2164,13 +2164,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }
 
-#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
+#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||  SNDRV_PCM_RATE_24000 != 1 << 15
 #error "Change this table"
 #endif
 
 static const unsigned int rates[] = {
 	5512, 8000, 11025, 16000, 22050, 32000, 44100,
-	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000
+	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 24000
 };
 
 const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [RFC PATCH 3/3] ALSA:hda: Add rates 24kHz, 64kHz

[Pawel Harlozinski]

Adds rates 24kHz & 64kHz to allow proper calculation SDxFMT value.

Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
---
 sound/hda/hdac_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index add758e11b85..72e00d93adb6 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -702,14 +702,16 @@ static struct hda_rate_tbl rate_bits[] = {
 	{ 11025, SNDRV_PCM_RATE_11025, HDA_RATE(44, 1, 4) },
 	{ 16000, SNDRV_PCM_RATE_16000, HDA_RATE(48, 1, 3) },
 	{ 22050, SNDRV_PCM_RATE_22050, HDA_RATE(44, 1, 2) },
+	{ 24000, SNDRV_PCM_RATE_24000, HDA_RATE(48, 1, 2) },
 	{ 32000, SNDRV_PCM_RATE_32000, HDA_RATE(48, 2, 3) },
 	{ 44100, SNDRV_PCM_RATE_44100, HDA_RATE(44, 1, 1) },
 	{ 48000, SNDRV_PCM_RATE_48000, HDA_RATE(48, 1, 1) },
+	{ 64000, SNDRV_PCM_RATE_64000, HDA_RATE(48, 4, 3) },
 	{ 88200, SNDRV_PCM_RATE_88200, HDA_RATE(44, 2, 1) },
 	{ 96000, SNDRV_PCM_RATE_96000, HDA_RATE(48, 2, 1) },
 	{ 176400, SNDRV_PCM_RATE_176400, HDA_RATE(44, 4, 1) },
 	{ 192000, SNDRV_PCM_RATE_192000, HDA_RATE(48, 4, 1) },
-#define AC_PAR_PCM_RATE_BITS	11
+#define AC_PAR_PCM_RATE_BITS	13
 	/* up to bits 10, 384kHZ isn't supported properly */
 
 	/* not autodetected value */
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [RFC PATCH 3/3] ALSA:hda: Add rates 24kHz, 64kHz

[Pawel Harlozinski]

Adds rates 24kHz & 64kHz to allow proper calculation SDxFMT value.

Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
---
 sound/hda/hdac_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index add758e11b85..72e00d93adb6 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -702,14 +702,16 @@ static struct hda_rate_tbl rate_bits[] = {
 	{ 11025, SNDRV_PCM_RATE_11025, HDA_RATE(44, 1, 4) },
 	{ 16000, SNDRV_PCM_RATE_16000, HDA_RATE(48, 1, 3) },
 	{ 22050, SNDRV_PCM_RATE_22050, HDA_RATE(44, 1, 2) },
+	{ 24000, SNDRV_PCM_RATE_24000, HDA_RATE(48, 1, 2) },
 	{ 32000, SNDRV_PCM_RATE_32000, HDA_RATE(48, 2, 3) },
 	{ 44100, SNDRV_PCM_RATE_44100, HDA_RATE(44, 1, 1) },
 	{ 48000, SNDRV_PCM_RATE_48000, HDA_RATE(48, 1, 1) },
+	{ 64000, SNDRV_PCM_RATE_64000, HDA_RATE(48, 4, 3) },
 	{ 88200, SNDRV_PCM_RATE_88200, HDA_RATE(44, 2, 1) },
 	{ 96000, SNDRV_PCM_RATE_96000, HDA_RATE(48, 2, 1) },
 	{ 176400, SNDRV_PCM_RATE_176400, HDA_RATE(44, 4, 1) },
 	{ 192000, SNDRV_PCM_RATE_192000, HDA_RATE(48, 4, 1) },
-#define AC_PAR_PCM_RATE_BITS	11
+#define AC_PAR_PCM_RATE_BITS	13
 	/* up to bits 10, 384kHZ isn't supported properly */
 
 	/* not autodetected value */
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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

Re: [alsa-devel] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Takashi Iwai]

On Thu, 05 Sep 2019 07:33:00 +0200,
Pawel Harlozinski wrote:
> 
> Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
> with alsa library.
> 
> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>

No.  Such a fancy rate has to be handled inside the driver locally
instead of adding to the common rate.


Takashi

> ---
>  include/sound/pcm.h     | 1 +
>  sound/core/pcm_native.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index bbe6eb1ff5d2..09d0a2a2dce8 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -119,6 +119,7 @@ struct snd_pcm_ops {
>  #define SNDRV_PCM_RATE_192000		(1<<12)		/* 192000Hz */
>  #define SNDRV_PCM_RATE_352800		(1<<13)		/* 352800Hz */
>  #define SNDRV_PCM_RATE_384000		(1<<14)		/* 384000Hz */
> +#define SNDRV_PCM_RATE_24000		(1<<15)		/* 24000Hz */
>  
>  #define SNDRV_PCM_RATE_CONTINUOUS	(1<<30)		/* continuous range */
>  #define SNDRV_PCM_RATE_KNOT		(1<<31)		/* supports more non-continuos rates */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 11e653c8aa0e..f52f28e3edb1 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2164,13 +2164,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
>  	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
>  }
>  
> -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
> +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||  SNDRV_PCM_RATE_24000 != 1 << 15
>  #error "Change this table"
>  #endif
>  
>  static const unsigned int rates[] = {
>  	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> -	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000
> +	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 24000
>  };
>  
>  const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
> -- 
> 2.17.1
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Pierre-Louis Bossart]

On 9/5/19 12:48 AM, Takashi Iwai wrote:
> On Thu, 05 Sep 2019 07:33:00 +0200,
> Pawel Harlozinski wrote:
>>
>> Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
>> with alsa library.
>>
>> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
> 
> No.  Such a fancy rate has to be handled inside the driver locally
> instead of adding to the common rate.

It's not that crazy, this is supported in the HDaudio spec:

Sample Base Rate Divisor (DIV):
000 = Divide by 1 (48 kHz, 44.1 kHz)
001 = Divide by 2 (24 kHz, 22.05 kHz)

I am not sure why 22.05 made the cut and not 24 kHz, they are both 
derived from common clocks with the same divider... Same for 11.025 and 
12...

> 
> 
> Takashi
> 
>> ---
>>   include/sound/pcm.h     | 1 +
>>   sound/core/pcm_native.c | 4 ++--
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index bbe6eb1ff5d2..09d0a2a2dce8 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -119,6 +119,7 @@ struct snd_pcm_ops {
>>   #define SNDRV_PCM_RATE_192000		(1<<12)		/* 192000Hz */
>>   #define SNDRV_PCM_RATE_352800		(1<<13)		/* 352800Hz */
>>   #define SNDRV_PCM_RATE_384000		(1<<14)		/* 384000Hz */
>> +#define SNDRV_PCM_RATE_24000		(1<<15)		/* 24000Hz */
>>   
>>   #define SNDRV_PCM_RATE_CONTINUOUS	(1<<30)		/* continuous range */
>>   #define SNDRV_PCM_RATE_KNOT		(1<<31)		/* supports more non-continuos rates */
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 11e653c8aa0e..f52f28e3edb1 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -2164,13 +2164,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
>>   	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
>>   }
>>   
>> -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
>> +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||  SNDRV_PCM_RATE_24000 != 1 << 15
>>   #error "Change this table"
>>   #endif
>>   
>>   static const unsigned int rates[] = {
>>   	5512, 8000, 11025, 16000, 22050, 32000, 44100,
>> -	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000
>> +	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 24000
>>   };
>>   
>>   const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
>> -- 
>> 2.17.1
>>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Takashi Iwai]

On Thu, 05 Sep 2019 15:11:43 +0200,
Pierre-Louis Bossart wrote:
> 
> On 9/5/19 12:48 AM, Takashi Iwai wrote:
> > On Thu, 05 Sep 2019 07:33:00 +0200,
> > Pawel Harlozinski wrote:
> >>
> >> Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
> >> with alsa library.
> >>
> >> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
> >
> > No.  Such a fancy rate has to be handled inside the driver locally
> > instead of adding to the common rate.
> 
> It's not that crazy, this is supported in the HDaudio spec:
> 
> Sample Base Rate Divisor (DIV):
> 000 = Divide by 1 (48 kHz, 44.1 kHz)
> 001 = Divide by 2 (24 kHz, 22.05 kHz)
> 
> I am not sure why 22.05 made the cut and not 24 kHz, they are both
> derived from common clocks with the same divider... Same for 11.025
> and 12...

I'm not against supporting it.  It'd be fine if it were the changes
that are applied only to HD-audio driver side.  What I'm against is to
change the ALSA PCM core.  It's not necessarily done there at all.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Pierre-Louis Bossart]

>>>> Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
>>>> with alsa library.
>>>>
>>>> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
>>>
>>> No.  Such a fancy rate has to be handled inside the driver locally
>>> instead of adding to the common rate.
>>
>> It's not that crazy, this is supported in the HDaudio spec:
>>
>> Sample Base Rate Divisor (DIV):
>> 000 = Divide by 1 (48 kHz, 44.1 kHz)
>> 001 = Divide by 2 (24 kHz, 22.05 kHz)
>>
>> I am not sure why 22.05 made the cut and not 24 kHz, they are both
>> derived from common clocks with the same divider... Same for 11.025
>> and 12...
> 
> I'm not against supporting it.  It'd be fine if it were the changes
> that are applied only to HD-audio driver side.  What I'm against is to
> change the ALSA PCM core.  It's not necessarily done there at all.

Humm, out of curiosity what is the issue here? Would this addition break 
anything? I don't personally care too much but I've never quite 
understood why the ALSA core only defined a subset of 'common' rates.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [RFC PATCH 2/3] ALSA:core: Add rate 24kHz

[Takashi Iwai]

On Thu, 05 Sep 2019 16:00:51 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>>> Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability
> >>>> with alsa library.
> >>>>
> >>>> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
> >>>
> >>> No.  Such a fancy rate has to be handled inside the driver locally
> >>> instead of adding to the common rate.
> >>
> >> It's not that crazy, this is supported in the HDaudio spec:
> >>
> >> Sample Base Rate Divisor (DIV):
> >> 000 = Divide by 1 (48 kHz, 44.1 kHz)
> >> 001 = Divide by 2 (24 kHz, 22.05 kHz)
> >>
> >> I am not sure why 22.05 made the cut and not 24 kHz, they are both
> >> derived from common clocks with the same divider... Same for 11.025
> >> and 12...
> >
> > I'm not against supporting it.  It'd be fine if it were the changes
> > that are applied only to HD-audio driver side.  What I'm against is to
> > change the ALSA PCM core.  It's not necessarily done there at all.
> 
> Humm, out of curiosity what is the issue here? Would this addition
> break anything? I don't personally care too much but I've never quite
> understood why the ALSA core only defined a subset of 'common' rates.

It's simply a policy that we don't add a thing just because one driver
wants for some reason possibly no one would actually use.

In general such a core stuff is changed only when it has to be and
inevitably necessary, and/or it'd be benefit for all the rest and
majority users.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Harlozinski, Pawel]

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

Re: [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@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