[PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Lars-Peter Clausen]

SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
expressed using the rate bits. The driver will provide a list of those rates
specified through constraints. Any rate bits that are set in the rates mask will
be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
effect, but might be confusing to the casual reader, so remove them.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/codecs/cs42l73.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/cs42l73.c b/sound/soc/codecs/cs42l73.c
index 549d5d6..7cae046 100644
--- a/sound/soc/codecs/cs42l73.c
+++ b/sound/soc/codecs/cs42l73.c
@@ -1255,9 +1255,6 @@ static int cs42l73_pcm_startup(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-/* SNDRV_PCM_RATE_KNOT -> 12000, 24000 Hz, limit with constraint list */
-#define CS42L73_RATES (SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_KNOT)
-
 
 #define CS42L73_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 	SNDRV_PCM_FMTBIT_S24_LE)
@@ -1278,14 +1275,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
 			.stream_name = "XSP Playback",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.capture = {
 			.stream_name = "XSP Capture",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.ops = &cs42l73_ops,
@@ -1298,14 +1295,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
 			.stream_name = "ASP Playback",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.capture = {
 			.stream_name = "ASP Capture",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.ops = &cs42l73_ops,
@@ -1318,14 +1315,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
 			.stream_name = "VSP Playback",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.capture = {
 			.stream_name = "VSP Capture",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = CS42L73_RATES,
+			.rates = SNDRV_PCM_RATE_KNOT,
 			.formats = CS42L73_FORMATS,
 		},
 		.ops = &cs42l73_ops,
-- 
1.8.0

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Austin, Brian]

> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
> 
> SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
> expressed using the rate bits. The driver will provide a list of those rates
> specified through constraints. Any rate bits that are set in the rates mask will
> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
> effect, but might be confusing to the casual reader, so remove them.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> sound/soc/codecs/cs42l73.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs42l73.c b/sound/soc/codecs/cs42l73.c
> index 549d5d6..7cae046 100644
> --- a/sound/soc/codecs/cs42l73.c
> +++ b/sound/soc/codecs/cs42l73.c
> @@ -1255,9 +1255,6 @@ static int cs42l73_pcm_startup(struct snd_pcm_substream *substream,
>    return 0;
> }
> 
> -/* SNDRV_PCM_RATE_KNOT -> 12000, 24000 Hz, limit with constraint list */
> -#define CS42L73_RATES (SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_KNOT)
> -
> 
> #define CS42L73_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
>    SNDRV_PCM_FMTBIT_S24_LE)
> @@ -1278,14 +1275,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
>            .stream_name = "XSP Playback",
>            .channels_min = 1,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .capture = {
>            .stream_name = "XSP Capture",
>            .channels_min = 1,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .ops = &cs42l73_ops,
> @@ -1298,14 +1295,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
>            .stream_name = "ASP Playback",
>            .channels_min = 2,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .capture = {
>            .stream_name = "ASP Capture",
>            .channels_min = 2,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .ops = &cs42l73_ops,
> @@ -1318,14 +1315,14 @@ static struct snd_soc_dai_driver cs42l73_dai[] = {
>            .stream_name = "VSP Playback",
>            .channels_min = 1,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .capture = {
>            .stream_name = "VSP Capture",
>            .channels_min = 1,
>            .channels_max = 2,
> -            .rates = CS42L73_RATES,
> +            .rates = SNDRV_PCM_RATE_KNOT,
>            .formats = CS42L73_FORMATS,
>        },
>        .ops = &cs42l73_ops,
> -- 
> 1.8.0
> 
> 
I don't see how this makes it any less confusing to the casual reader. I think the define works well in the descriptive sense. maybe a comment about what KNOT really means instead of just using a non-rate define?

Thanks,
Brian 

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Lars-Peter Clausen]

On 02/04/2014 09:30 PM, Austin, Brian wrote:
>> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>>
>> SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
>> expressed using the rate bits. The driver will provide a list of those rates
>> specified through constraints. Any rate bits that are set in the rates mask will
>> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
>> effect, but might be confusing to the casual reader, so remove them.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
 >
> I don't see how this makes it any less confusing to the casual reader. I think the
 > define works well in the descriptive sense. maybe a comment about what 
KNOT really
 > means instead of just using a non-rate define?

The thing is SNDRV_PCM_RATE_8000_48000 does something if 
SNDRV_PCM_RATE_{KNOT,CONTINUOUS} is not set, but does nothing at all if 
either of them is set. While when reading the code it will, in my opinion, 
give the impression that the rates that are specified by 
SNDRV_PCM_RATE_8000_48000 are definitely among the supported rates and 
somewhere else additional rates are specified as well. But this is not the 
case all the supported rates need to be specified somewhere else, e.g. by a 
rate list constraint. I find this confusing when casually reading the code. 
And this is the only driver that does it.

Also there was a bug, which is now fixed, where additionally specifying 
rates in the rate bitmap when KNOT or CONTINUOUS was set did something, but 
did the wrong thing and caused additional rates which where outside of the 
range of the rates set in the bitmap to be ignored. Better avoid that by not 
mixing KNOT or CONTINUOUS with other rate bits.

- Lars

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Eliot Blennerhassett]

On 05/02/14 10:01, Lars-Peter Clausen wrote:
> e.g. by a rate list constraint. I find this confusing when casually
> reading the code. 


> And this is the only driver that does it.

A quick grep through the sound tree for "KNOT" shows that there are many
drivers that do?

Just a example:

pci/intel8x0m.c:  .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
SNDRV_PCM_RATE_KNOT


Do these need 'fixing'?

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Takashi Iwai]

At Wed, 05 Feb 2014 10:49:06 +1300,
Eliot Blennerhassett wrote:
> 
> On 05/02/14 10:01, Lars-Peter Clausen wrote:
> > e.g. by a rate list constraint. I find this confusing when casually
> > reading the code. 
> 
> 
> > And this is the only driver that does it.
> 
> A quick grep through the sound tree for "KNOT" shows that there are many
> drivers that do?
> 
> Just a example:
> 
> pci/intel8x0m.c:  .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
> SNDRV_PCM_RATE_KNOT
> 
> 
> Do these need 'fixing'?

It would be no "fix" but rather a "cleanup".


Takashi

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Takashi Iwai]

At Wed, 05 Feb 2014 10:49:06 +1300,
Eliot Blennerhassett wrote:
> 
> On 05/02/14 10:01, Lars-Peter Clausen wrote:
> > e.g. by a rate list constraint. I find this confusing when casually
> > reading the code. 
> 
> 
> > And this is the only driver that does it.
> 
> A quick grep through the sound tree for "KNOT" shows that there are many
> drivers that do?
> 
> Just a example:
> 
> pci/intel8x0m.c:  .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
> SNDRV_PCM_RATE_KNOT
> 
> 
> Do these need 'fixing'?

It would be no "fix" but rather a "cleanup".


Takashi

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Austin, Brian]

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


On Feb 4, 2014, at 3:01 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 02/04/2014 09:30 PM, Austin, Brian wrote:
>>> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>>> 
>>> SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
>>> expressed using the rate bits. The driver will provide a list of those rates
>>> specified through constraints. Any rate bits that are set in the rates mask will
>>> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
>>> effect, but might be confusing to the casual reader, so remove them.
>>> 
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> >
>> I don't see how this makes it any less confusing to the casual reader. I think the
> > define works well in the descriptive sense. maybe a comment about what KNOT really
> > means instead of just using a non-rate define?
> 
> The thing is SNDRV_PCM_RATE_8000_48000 does something if SNDRV_PCM_RATE_{KNOT,CONTINUOUS} is not set, but does nothing at all if either of them is set.


> While when reading the code it will, in my opinion, give the impression that the rates that are
> specified by SNDRV_PCM_RATE_8000_48000 are definitely among the supported rates and somewhere else additional rates are specified as well. But this is not the case all the supported rates need to be specified somewhere else, e.g. by a rate list constraint. I find this confusing when casually reading the code. And this is the only driver that does it.
But that is the case is it not? The device supports the rates within 8000 and 48000. There are additional rates that need to be specified. If anything I see the issue with having to specify ALL the rates supported in the constraint list. The KNOT is an addition to the .rates member. The KNOT list is added to the .rates member in the dai driver.

> 
> Also there was a bug, which is now fixed, where additionally specifying rates in the rate bitmap when KNOT or CONTINUOUS was set did something, but did the wrong thing and caused additional rates which where outside of the range of the rates set in the bitmap to be ignored. Better avoid that by not mixing KNOT or CONTINUOUS with other rate bits.
> 
Shouldn’t the rates outside of the range of the rates bitmap be ignored?  Maybe this is a wrong interpretation of how this is supposed to work, but I thought you specify the rates that are available and add a constraint list to add rates that are not defined in ALSA. Not that the KNOT value would mean that you also have rates outside of the values you specified.

example: SNDRV_PCM_RATE_8000_48000 | | SNDRV_PCM_RATE_KNOT means that I support all rates defined between 8 and 48 PLUS addition rates INSIDE of 8 and 48.  Is this correct?

Sorry Lars, I just want to have a clear understanding of what the bug is and how this fixes it.

Thanks,
Brian

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]

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

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Lars-Peter Clausen]

On 02/05/2014 03:35 PM, Austin, Brian wrote:
>
> On Feb 4, 2014, at 3:01 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> On 02/04/2014 09:30 PM, Austin, Brian wrote:
>>>> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>>>>
>>>> SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
>>>> expressed using the rate bits. The driver will provide a list of those rates
>>>> specified through constraints. Any rate bits that are set in the rates mask will
>>>> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
>>>> effect, but might be confusing to the casual reader, so remove them.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>
>>> I don't see how this makes it any less confusing to the casual reader. I think the
>>> define works well in the descriptive sense. maybe a comment about what KNOT really
>>> means instead of just using a non-rate define?
>>
>> The thing is SNDRV_PCM_RATE_8000_48000 does something if SNDRV_PCM_RATE_{KNOT,CONTINUOUS} is not set, but does nothing at all if either of them is set.
>
>
>> While when reading the code it will, in my opinion, give the impression that the rates that are
>> specified by SNDRV_PCM_RATE_8000_48000 are definitely among the supported rates and somewhere else additional rates are specified as well. But this is not the case all the supported rates need to be specified somewhere else, e.g. by a rate list constraint. I find this confusing when casually reading the code. And this is the only driver that does it.
> But that is the case is it not? The device supports the rates within 8000 and 48000. There are additional rates that need to be specified. If anything I see the issue with having to specify ALL the rates supported in the constraint list. The KNOT is an addition to the .rates member. The KNOT list is added to the .rates member in the dai driver.
>

Yep, but that's not how it works and why I think it is confusing to keep 
things the SNDRV_PCM_RATE_8000_48000 in there, as it might cause people to 
think that this is how it works.

When a substream is opened the constraints are set to be as permissive as 
possible. E.g. for rates this means any rate is considered valid. Going from 
there the constraints code allows you to further restrict things. This is 
done by attaching rules to the constraint. The result is the intersection of 
all the rules attached to the constraint. E.g. if you have one rule that 
says the rate must be between 16000 and 64000 and another rule that says the 
rate must be one of 8000, 16000, 32000 or 48000, the list of valid rates is 
16000, 32000, 48000. Similar if you have one rule that says the rate must be 
one of 8000, 16000 or 32000 and  another rule that says that the rate must 
be one of 16000, 22500 or 24000, the list of valid rates is 16000. It is 
only possible to further restrict the set of supported rates by one rule, it 
is not possible to make it more permissive.

When you set the rates field (and omit the KNOT or CONTINUOUS flags) the 
ALSA core will install one rule that will limit the supported rates to those 
set in the rates field. When you use snd_pcm_hw_constraint_list() this will 
add a second rule that will only allow rates in the array you specified for 
that constraint. So the result will be that only those rates that are 
allowed by both rules.

That's why you need to specify all supported rates in the constraint. Note 
that ALSA core will do nothing with the rates field if the KNOT or 
CONTINUOUS flag is set. Otherwise you wouldn't be able to specify rates 
outside of what can be specified using the standard rates.

>>
>> Also there was a bug, which is now fixed, where additionally specifying rates in the rate bitmap when KNOT or CONTINUOUS was set did something, but did the wrong thing and caused additional rates which where outside of the range of the rates set in the bitmap to be ignored. Better avoid that by not mixing KNOT or CONTINUOUS with other rate bits.
>>
> Shouldn’t the rates outside of the range of the rates bitmap be ignored?  Maybe this is a wrong interpretation of how this is supposed to work, but I thought you specify the rates that are available and add a constraint list to add rates that are not defined in ALSA. Not that the KNOT value would mean that you also have rates outside of the values you specified.
>

If you ignore rates outside the range of what is specified in the rates 
field you wouldn't be able to use rates that are lower or higher than what 
is possible to specify with the default rates.

> example: SNDRV_PCM_RATE_8000_48000 | | SNDRV_PCM_RATE_KNOT means that I support all rates defined between 8 and 48 PLUS addition rates INSIDE of 8 and 48.  Is this correct?
>

It means you support some rates that need to be further specified somewhere 
else. If SNDRV_PCM_RATE_KNOT (or SNDRV_PCM_RATE_CONTINUOUS) is set in the 
rates field all other bits that might be set are fully ignored (And that's 
not a bug, it's a feature).

> Sorry Lars, I just want to have a clear understanding of what the bug is and how this fixes it.
>
> Thanks,
> Brian
>

There is no bug (anymore). I already fixed things up in the ALSA and ASoC 
core so that if KNOT or CONTINUOUS is set all the other bits are masked out 
from the rates field. Before that was done there was a bug that for some 
drivers rates were ignored that should have been valid. This patch is a 
cleanup that removes a code fragment that on the first look suggests that it 
does something while it actually does nothing. And I think you proved my 
point that this can be confusing and misleading :)

- Lars

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Austin, Brian]

On 2/5/14, 9:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:

>On 02/05/2014 03:35 PM, Austin, Brian wrote:
>>
>> On Feb 4, 2014, at 3:01 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> On 02/04/2014 09:30 PM, Austin, Brian wrote:
>>>>> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars@metafoo.de>
>>>>>wrote:
>>>>>
>>>>> SNDRV_PCM_RATE_KNOT means that the device can support rates that can
>>>>>not be
>>>>> expressed using the rate bits. The driver will provide a list of
>>>>>those rates
>>>>> specified through constraints. Any rate bits that are set in the
>>>>>rates mask will
>>>>> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT
>>>>>wont have any
>>>>> effect, but might be confusing to the casual reader, so remove them.
>>>>>
>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>
>>>> I don't see how this makes it any less confusing to the casual
>>>>reader. I think the
>>>> define works well in the descriptive sense. maybe a comment about
>>>>what KNOT really
>>>> means instead of just using a non-rate define?
>>>
>>> The thing is SNDRV_PCM_RATE_8000_48000 does something if
>>>SNDRV_PCM_RATE_{KNOT,CONTINUOUS} is not set, but does nothing at all if
>>>either of them is set.
>>
>>
>>> While when reading the code it will, in my opinion, give the
>>>impression that the rates that are
>>> specified by SNDRV_PCM_RATE_8000_48000 are definitely among the
>>>supported rates and somewhere else additional rates are specified as
>>>well. But this is not the case all the supported rates need to be
>>>specified somewhere else, e.g. by a rate list constraint. I find this
>>>confusing when casually reading the code. And this is the only driver
>>>that does it.
>> But that is the case is it not? The device supports the rates within
>>8000 and 48000. There are additional rates that need to be specified. If
>>anything I see the issue with having to specify ALL the rates supported
>>in the constraint list. The KNOT is an addition to the .rates member.
>>The KNOT list is added to the .rates member in the dai driver.
>>
>
>Yep, but that's not how it works and why I think it is confusing to keep
>things the SNDRV_PCM_RATE_8000_48000 in there, as it might cause people
>to 
>think that this is how it works.
>
>When a substream is opened the constraints are set to be as permissive as
>possible. E.g. for rates this means any rate is considered valid. Going
>from 
>there the constraints code allows you to further restrict things. This is
>done by attaching rules to the constraint. The result is the intersection
>of 
>all the rules attached to the constraint. E.g. if you have one rule that
>says the rate must be between 16000 and 64000 and another rule that says
>the 
>rate must be one of 8000, 16000, 32000 or 48000, the list of valid rates
>is 
>16000, 32000, 48000. Similar if you have one rule that says the rate must
>be 
>one of 8000, 16000 or 32000 and  another rule that says that the rate
>must 
>be one of 16000, 22500 or 24000, the list of valid rates is 16000. It is
>only possible to further restrict the set of supported rates by one rule,
>it 
>is not possible to make it more permissive.
>
>When you set the rates field (and omit the KNOT or CONTINUOUS flags) the
>ALSA core will install one rule that will limit the supported rates to
>those 
>set in the rates field. When you use snd_pcm_hw_constraint_list() this
>will 
>add a second rule that will only allow rates in the array you specified
>for 
>that constraint. So the result will be that only those rates that are
>allowed by both rules.
>
>That's why you need to specify all supported rates in the constraint.
>Note 
>that ALSA core will do nothing with the rates field if the KNOT or
>CONTINUOUS flag is set. Otherwise you wouldn't be able to specify rates
>outside of what can be specified using the standard rates.
>
>>>
>>> Also there was a bug, which is now fixed, where additionally
>>>specifying rates in the rate bitmap when KNOT or CONTINUOUS was set did
>>>something, but did the wrong thing and caused additional rates which
>>>where outside of the range of the rates set in the bitmap to be
>>>ignored. Better avoid that by not mixing KNOT or CONTINUOUS with other
>>>rate bits.
>>>
>> Shouldn¹t the rates outside of the range of the rates bitmap be
>>ignored?  Maybe this is a wrong interpretation of how this is supposed
>>to work, but I thought you specify the rates that are available and add
>>a constraint list to add rates that are not defined in ALSA. Not that
>>the KNOT value would mean that you also have rates outside of the values
>>you specified.
>>
>
>If you ignore rates outside the range of what is specified in the rates
>field you wouldn't be able to use rates that are lower or higher than
>what 
>is possible to specify with the default rates.
>
>> example: SNDRV_PCM_RATE_8000_48000 | | SNDRV_PCM_RATE_KNOT means that I
>>support all rates defined between 8 and 48 PLUS addition rates INSIDE of
>>8 and 48.  Is this correct?
>>
>
>It means you support some rates that need to be further specified
>somewhere 
>else. If SNDRV_PCM_RATE_KNOT (or SNDRV_PCM_RATE_CONTINUOUS) is set in the
>rates field all other bits that might be set are fully ignored (And
>that's 
>not a bug, it's a feature).
>
>> Sorry Lars, I just want to have a clear understanding of what the bug
>>is and how this fixes it.
>>
>> Thanks,
>> Brian
>>
>
>There is no bug (anymore). I already fixed things up in the ALSA and ASoC
>core so that if KNOT or CONTINUOUS is set all the other bits are masked
>out 
>from the rates field. Before that was done there was a bug that for some
>drivers rates were ignored that should have been valid. This patch is a
>cleanup that removes a code fragment that on the first look suggests that
>it 
>does something while it actually does nothing. And I think you proved my
>point that this can be confusing and misleading :)
>
>- Lars
>
Thanks for the explanation. I had a reverse understanding of what this
actually does for the dai driver .rates coupled with the constraint list.

Thanks,
Brian

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Austin, Brian]

On 2/4/14, 1:55 PM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:

>SNDRV_PCM_RATE_KNOT means that the device can support rates that can not
>be
>expressed using the rate bits. The driver will provide a list of those
>rates
>specified through constraints. Any rate bits that are set in the rates
>mask will
>be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont
>have any
>effect, but might be confusing to the casual reader, so remove them.
>
>Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>---
Acked-by: Brian Austin <brian.austin@cirrus.com>

Re: [PATCH] ASoC: cs42l73: Don't mix SNDRV_PCM_RATE_KNOT with specific rates

[Mark Brown]

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

On Tue, Feb 04, 2014 at 08:55:31PM +0100, Lars-Peter Clausen wrote:
> SNDRV_PCM_RATE_KNOT means that the device can support rates that can not be
> expressed using the rate bits. The driver will provide a list of those rates
> specified through constraints. Any rate bits that are set in the rates mask will
> be ignored. So setting other rate bits besides SNDRV_PCM_RATE_KNOT wont have any
> effect, but might be confusing to the casual reader, so remove them.

Applied, thanks.

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

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