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

[Austin, Brian]

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

> On 02/04/2014 09:30 PM, Austin, Brian wrote:
>>> On Feb 4, 2014, at 13:55, "Lars-Peter Clausen" <lars at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140205/d9d60159/attachment-0001.sig>