Re: [alsa-devel] [PATCH 3/5] ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks

On Fri, 2017-09-08 at 12:43 -0500, Pierre-Louis Bossart wrote:

Same as for other codecs, enable MCLK by default. When it is not present, e.g. on MinnowBoard B3 since it's not routed on the LSE connector, we fall back to blck-based clocking.

The DMIC quirks are also fixed, there is a single DMIC input of the codec

#include <linux/device.h> #include <linux/dmi.h> #include <linux/slab.h> +#include <asm/cpu_device_id.h> +#include <asm/platform_sst_audio.h>

+#include <linux/clk.h>

Just a nit: I would rather squeeze this to somewhere above device (alphabetical ordering)

#include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h>

+#define BYT_RT5651_MAP(quirk) ((quirk) & 0xff)

GENMASK(7, 0) ?

+#define BYT_RT5651_DMIC_EN BIT(16) +#define BYT_RT5651_MCLK_EN BIT(17) +#define BYT_RT5651_MCLK_25MHZ BIT(18)

+static void log_quirks(struct device *dev) +{

• if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_DMIC_MAP)

• dev_info(dev, "quirk DMIC_MAP enabled");
  

• if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)

• dev_info(dev, "quirk IN1_MAP enabled");
  

• if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)

• dev_info(dev, "quirk DMIC enabled");
  

• if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)

• dev_info(dev, "quirk MCLK_EN enabled");
  

• if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)

• dev_info(dev, "quirk MCLK_25MHZ enabled");
  

+}

+#define BYT_CODEC_DAI1 "rt5651-aif1"

+static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card) +{

• struct snd_soc_pcm_runtime *rtd;
• list_for_each_entry(rtd, &card->rtd_list, list) {

• if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI1,
  

• 	     strlen(BYT_CODEC_DAI1)))
  

Still same comment about str_n_cmp vs. strcmp() in this case.

(strlen() also can be calculated once size_t dai1_len = strlen(...); )

• 	return rtd->codec_dai;
  

• }
• return NULL;

• if (SND_SOC_DAPM_EVENT_ON(event)) {

• if (priv->mclk) {
  

Do we need this check?

I'm not sure I have read a comment why this is left as in initial series.

		     SND_SOC_CLOCK_IN);

• if (!ret)
  

• 	if (priv->mclk)
  

Ditto ?

• 		clk_disable_unprepare(priv->mclk);
  

• }

static const struct dmi_system_id byt_rt5651_quirk_table[] = {

• {

• .callback = byt_rt5651_quirk_cb,
  

• .matches = {
  

• 	DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"),
  

• 	DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Max
  

B3 PLATFORM"),

• },
  

• .driver_data = (unsigned long *)(BYT_RT5651_DMIC_MAP
  

|

Shouldn't be just (void *) [and maybe fit one line]?

• 				 BYT_RT5651_DMIC_EN),
  

• if (priv->mclk) {

Same question as above.

• /*
  

•  * The firmware might enable the clock at
  

•  * boot (this information may or may not
  

•  * be reflected in the enable clock register).
  

•  * To change the rate we must disable the clock
  

•  * first to cover these cases. Due to common
  

•  * clock framework restrictions that do not allow
  

•  * to disable a clock that has not been enabled,
  

•  * we need to enable the clock first.
  

•  */
  

• ret = clk_prepare_enable(priv->mclk);
  

• if (!ret)
  

• 	clk_disable_unprepare(priv->mclk);
  

• if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
  

• 	ret = clk_set_rate(priv->mclk, 25000000);
  

• else
  

• 	ret = clk_set_rate(priv->mclk, 19200000);
  

• if (ret)
  

• 	dev_err(card->dev, "unable to set MCLK
  

rate\n");

• }