[alsa-devel] [PATCH 2/2] ASoC: pcm: Always honor DAI min and max sample rate constraints

[Takashi Iwai]

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