On 28/02/2018 at 11:23:13 +0100, Ladislav Michl wrote:
> On Wed, Feb 28, 2018 at 11:00:54AM +0100, Alexandre Belloni wrote:
> > On 27/02/2018 at 20:03:38 +0100, Ladislav Michl wrote:
> > > On Tue, Feb 27, 2018 at 06:23:09PM +0100, Alexandre Belloni wrote:
> > > > This also needs a proper commit message.
> > >
> > > Meanwhile more patches for this driver accumulated, so I'll send simple ones
> > > first and those I'm uncertain of later.
> > >
> > > Now few questions:
> > > - should hw_params emit any error message when asked to set for example
> > > unsupported sample rate? Many drivers do so, but it seems strange.
> >
> > I would thin that is correct because then the eror goes up to userspace
> > and it knows it has to resample.
>
> Perhaps I didn't express it clearly enough. I mean all those
> dev_err(dev, "sunsupported rate\n");
> Here only human sitting in realspace is able to react somehow, but I'd say
> he's only annoyed.
>
> I would expect driver is able to set any rate it is advertising, so returned
> -EINVAL is considered to be a programming error (which shouldn't clutter
> kernel log).
>
Ah yes, please feel free to remove the dev_err messages. I also find
that they clutter the kernel output needlessly.
> > > - table used in this driver is overkill, frequencies can be calculated
> > > directly (patch ready), but that brings question about
> > > SNDRV_PCM_RATE_CONTINUOUS: what does it exactly mean? What if codec is
> > > able to set "any" rate, but there are rounding errors?
> >
> > That is a good question I can't answer.
>
> Thank you anyway, hopefully someone else would be able to answer.
>
Well, Mark is not Cced, you'd have to get his attention somehow ;)
> Consider patch bellow - with introduced change codec is able to set any rate
> in master mode (it is an example only, it needs more work), but not any rate
> will be matched exactly. Of course, for each master clock rate it is possible
> to compute exact supported rates and advertise them. Is that way forward or
> is there any trick with SNDRV_PCM_RATE_CONTINUOUS?
>
> Subject: ASoC: max9867: Use continuous sample rate
>
> Drop "Common NI Values Table" and calculate LRCLK divider.
>
> Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
>
> diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> index 0a6bd9f4d00a..d9b81a95a200 100644
> --- a/sound/soc/codecs/max9867.c
> +++ b/sound/soc/codecs/max9867.c
> @@ -126,75 +126,19 @@ static const struct snd_soc_dapm_route max9867_audio_map[] = {
> {"LINE_IN", NULL, "Right Line"},
> };
>
> -enum rates {
> - pcm_rate_8, pcm_rate_16, pcm_rate_24,
> - pcm_rate_32, pcm_rate_44,
> - pcm_rate_48, max_pcm_rate,
> -};
> -
> -static const struct ni_div_rates {
> - u32 mclk;
> - u16 ni[max_pcm_rate];
> -} ni_div[] = {
> - {11289600, {0x116A, 0x22D4, 0x343F, 0x45A9, 0x6000, 0x687D} },
> - {12000000, {0x1062, 0x20C5, 0x3127, 0x4189, 0x5A51, 0x624E} },
> - {12288000, {0x1000, 0x2000, 0x3000, 0x4000, 0x5833, 0x6000} },
> - {13000000, {0x0F20, 0x1E3F, 0x2D5F, 0x3C7F, 0x535F, 0x5ABE} },
> - {19200000, {0x0A3D, 0x147B, 0x1EB8, 0x28F6, 0x3873, 0x3D71} },
> - {24000000, {0x1062, 0x20C5, 0x1893, 0x4189, 0x5A51, 0x624E} },
> - {26000000, {0x0F20, 0x1E3F, 0x16AF, 0x3C7F, 0x535F, 0x5ABE} },
> - {27000000, {0x0E90, 0x1D21, 0x15D8, 0x3A41, 0x5048, 0x5762} },
> -};
> -
> -static inline int get_ni_value(int mclk, int rate)
> -{
> - int i, ret = 0;
> -
> - /* find the closest rate index*/
> - for (i = 0; i < ARRAY_SIZE(ni_div); i++) {
> - if (ni_div[i].mclk >= mclk)
> - break;
> - }
> - if (i == ARRAY_SIZE(ni_div))
> - return -EINVAL;
> -
> - switch (rate) {
> - case 8000:
> - return ni_div[i].ni[pcm_rate_8];
> - case 16000:
> - return ni_div[i].ni[pcm_rate_16];
> - case 32000:
> - return ni_div[i].ni[pcm_rate_32];
> - case 44100:
> - return ni_div[i].ni[pcm_rate_44];
> - case 48000:
> - return ni_div[i].ni[pcm_rate_48];
> - default:
> - pr_err("%s wrong rate %d\n", __func__, rate);
> - ret = -EINVAL;
> - }
> - return ret;
> -}
> -
> static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> {
> struct snd_soc_component *component = dai->component;
> struct max9867_priv *max9867 = snd_soc_component_get_drvdata(component);
> - unsigned int ni_h, ni_l;
> - int value;
> + unsigned int ni = DIV_ROUND_CLOSEST_ULL(96ULL * 0x10000 * params_rate(params),
> + max9867->pclk);
>
> - value = get_ni_value(max9867->sysclk, params_rate(params));
> - if (value < 0)
> - return value;
> -
> - ni_h = (0xFF00 & value) >> 8;
> - ni_l = 0x00FF & value;
> /* set up the ni value */
> regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
> - MAX9867_NI_HIGH_MASK, ni_h);
> + MAX9867_NI_HIGH_MASK, (0xFF00 & ni) >> 8);
> regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKLOW,
> - MAX9867_NI_LOW_MASK, ni_l);
> + MAX9867_NI_LOW_MASK, 0x00FF & ni);
> if (!max9867->master) {
> /*
> * digital pll locks on to any externally supplied LRCLK signal
> @@ -241,13 +185,13 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> /* Set the prescaler based on the master clock frequency*/
> if (freq >= 10000000 && freq <= 20000000) {
> value |= MAX9867_PSCLK_10_20;
> - max9867->pclk = freq;
> + max9867->pclk = freq;
> } else if (freq >= 20000000 && freq <= 40000000) {
> value |= MAX9867_PSCLK_20_40;
> - max9867->pclk = freq/2;
> + max9867->pclk = freq / 2;
> } else if (freq >= 40000000 && freq <= 60000000) {
> value |= MAX9867_PSCLK_40_60;
> - max9867->pclk = freq/4;
> + max9867->pclk = freq / 4;
> } else {
> dev_err(component->dev,
> "Invalid clock frequency %uHz (required 10-60MHz)\n",
> @@ -322,10 +266,6 @@ static const struct snd_soc_dai_ops max9867_dai_ops = {
> .hw_params = max9867_dai_hw_params,
> };
>
> -#define MAX9867_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |\
> - SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)
> -#define MAX9867_FORMATS (SNDRV_PCM_FMTBIT_S16_LE)
> -
> static struct snd_soc_dai_driver max9867_dai[] = {
> {
> .name = "max9867-aif1",
> @@ -333,15 +273,19 @@ static struct snd_soc_dai_driver max9867_dai[] = {
> .stream_name = "HiFi Playback",
> .channels_min = 1,
> .channels_max = 2,
> - .rates = MAX9867_RATES,
> - .formats = MAX9867_FORMATS,
> + .rates = SNDRV_PCM_RATE_CONTINUOUS,
> + .rate_min = 8000,
> + .rate_max = 48000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> },
> .capture = {
> .stream_name = "HiFi Capture",
> .channels_min = 1,
> .channels_max = 2,
> - .rates = MAX9867_RATES,
> - .formats = MAX9867_FORMATS,
> + .rates = SNDRV_PCM_RATE_CONTINUOUS,
> + .rate_min = 8000,
> + .rate_max = 48000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> },
> .ops = &max9867_dai_ops,
> }
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
