At Wed, 27 Nov 2013 12:21:53 +0000, Mark Brown wrote:
On Wed, Nov 27, 2013 at 12:26:29PM +0100, Takashi Iwai wrote:
Lars-Peter Clausen wrote:
My thinking as well. There are some driver which do set CONTINUOUS or KNOT, but don't specify a minimum and maximum rate and probably rely on the fact that we always call snd_pcm_limit_hw_rates() (E.g. the kirkwood drivers do this). In my opinion those drivers should be fixed and once that is done we can add a check that skips snd_pcm_limit_hw_rates() if KNOT or CONTINUOUS is set. The current behavior isn't optimal e.g. if a driver sets SNDRV_PCM_RATE_CONTINUOUS and SNDRV_PCM_RATE_8000_192000 and rate_max to 384000, we'd still end up with a maximum rate of 192000 because of snd_pcm_limit_hw_rates().
Right. We should catch all them once, then add a sanity check something like
It seems neater if snd_pcm_limit_hw_rates() does the right thing no matter what the rates are - this centralises the logic for handling this which reduces the complexity for drivers.
Well, the difficult point is to decide what is right, if the given condition is already wrong (e.g. rate_min/max don't match with the RATE bits). We can accept it somehow silently (as of now), but I think it's better to warn (and/or refuse).
WARN_ON((rates & SNDRV_PCM_RATE_KNOT) && \ (rates & ~SNDRV_PCM_RATE_KNOT)); WARN_ON((rates & SNDRV_PCM_RATE_CONTINUOUS) && \ (rates & ~SNDRV_PCM_RATE_CONTINUOUS));
This does make sense, though we'll need to change the constraint merging code in ASoC since right now it assumes that combining the masks will do the merge which won't be the case if you have devices with one of these flags together with any other flags (either specific rates or the other non-bitmask one). It would be nice if we were just throwing the device constraints into the core constraint code and letting it merge them together.
I think Lars' fix will handle such a case nicely now; at least, merging between RATE bits and rate_min/max is solved, as long as the given conditions are consistent.
But yes, we can certainly put some sanity checks and more handling code into a core stuff. But the drivers giving wrong parameters must be fixed in anyway. IIRC, even ac97_codec.c does something misaligned. Warnings would be a help for hunting such ones, and I didn't intend it being more than that in WARN_ON() above.
thanks,
Takashi