[PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Ban Tao]

The Allwinner H6 and later SoCs have an DMIC block
which is capable of capture.

Signed-off-by: Ban Tao <fengzheng923@gmail.com>
---
 MAINTAINERS                   |   7 +
 sound/soc/sunxi/Kconfig       |   8 +
 sound/soc/sunxi/Makefile      |   1 +
 sound/soc/sunxi/sun50i-dmic.c | 408 ++++++++++++++++++++++++++++++++++
 4 files changed, 424 insertions(+)
 create mode 100644 sound/soc/sunxi/sun50i-dmic.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b4094f07214e..1b87225c39b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -760,6 +760,13 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/staging/media/sunxi/cedrus/
 
+ALLWINNER DMIC DRIVERS
+M:	Ban Tao <fengzheng923@gmail.com>
+L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
+F:	sound/soc/sunxi/sun50i-dmic.c
+
 ALPHA PORT
 M:	Richard Henderson <rth@twiddle.net>
 M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
index ddcaaa98d3cb..2a3bf7722e11 100644
--- a/sound/soc/sunxi/Kconfig
+++ b/sound/soc/sunxi/Kconfig
@@ -56,6 +56,14 @@ config SND_SUN4I_SPDIF
 	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
 	  A10 and affiliated SoCs.
 
+config SND_SUN50I_DMIC
+	tristate "Allwinner H6 DMIC Support"
+	depends on (OF && ARCH_SUNXI) || COMPILE_TEST
+	select SND_SOC_GENERIC_DMAENGINE_PCM
+	help
+	  Say Y or M to add support for the DMIC audio block in the Allwinner
+	  H6 and affiliated SoCs.
+
 config SND_SUN8I_ADDA_PR_REGMAP
 	tristate
 	select REGMAP
diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
index a86be340a076..4483fe9c94ef 100644
--- a/sound/soc/sunxi/Makefile
+++ b/sound/soc/sunxi/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
 obj-$(CONFIG_SND_SUN50I_CODEC_ANALOG) += sun50i-codec-analog.o
 obj-$(CONFIG_SND_SUN8I_CODEC) += sun8i-codec.o
 obj-$(CONFIG_SND_SUN8I_ADDA_PR_REGMAP) += sun8i-adda-pr-regmap.o
+obj-$(CONFIG_SND_SUN50I_DMIC) += sun50i-dmic.o
diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
new file mode 100644
index 000000000000..78d512d93974
--- /dev/null
+++ b/sound/soc/sunxi/sun50i-dmic.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ALSA SoC DMIC Audio Layer
+ *
+ * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+
+#define SUN50I_DMIC_EN_CTL		(0x00)
+	#define SUN50I_DMIC_EN_CTL_GLOBE			BIT(8)
+	#define SUN50I_DMIC_EN_CTL_CHAN(v)		((v) << 0)
+	#define SUN50I_DMIC_EN_CTL_CHAN_MASK		GENMASK(7, 0)
+#define SUN50I_DMIC_SR			(0x04)
+	#define SUN50I_DMIC_SR_SAMOLE_RATE(v)		((v) << 0)
+	#define SUN50I_DMIC_SR_SAMOLE_RATE_MASK		GENMASK(2, 0)
+#define SUN50I_DMIC_CTL			(0x08)
+	#define SUN50I_DMIC_CTL_OVERSAMPLE_RATE		BIT(0)
+#define SUN50I_DMIC_DATA			(0x10)
+#define SUN50I_DMIC_INTC			(0x14)
+	#define SUN50I_DMIC_FIFO_DRQ_EN			BIT(2)
+#define SUN50I_DMIC_INT_STA		(0x18)
+	#define SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING	BIT(1)
+	#define SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING	BIT(0)
+#define SUN50I_DMIC_RXFIFO_CTL		(0x1c)
+	#define SUN50I_DMIC_RXFIFO_CTL_FLUSH		BIT(31)
+	#define SUN50I_DMIC_RXFIFO_CTL_MODE		BIT(9)
+	#define SUN50I_DMIC_RXFIFO_CTL_RESOLUTION	BIT(8)
+#define SUN50I_DMIC_CH_NUM		(0x24)
+	#define SUN50I_DMIC_CH_NUM_N(v)			((v) << 0)
+	#define SUN50I_DMIC_CH_NUM_N_MASK		GENMASK(2, 0)
+#define SUN50I_DMIC_CNT			(0x2c)
+	#define SUN50I_DMIC_CNT_N			BIT(0)
+#define SUN50I_DMIC_HPF_CTRL		(0x38)
+#define SUN50I_DMIC_VERSION		(0x50)
+
+
+struct sun50i_dmic_dev {
+	struct platform_device *pdev;
+	struct clk *dmic_clk;
+	struct clk *apb_clk;
+	struct reset_control *rst;
+	struct regmap *regmap;
+	struct snd_dmaengine_dai_dma_data dma_params_rx;
+	unsigned int chan_en;
+};
+
+struct dmic_rate {
+	unsigned int samplerate;
+	unsigned int rate_bit;
+};
+
+static void sun50i_snd_rxctrl_enable(struct snd_pcm_substream *substream,
+				    struct sun50i_dmic_dev *host, bool enable)
+{
+	if (enable) {
+		/* DRQ ENABLE */
+		regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
+				   SUN50I_DMIC_FIFO_DRQ_EN,
+				   SUN50I_DMIC_FIFO_DRQ_EN);
+		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
+				   SUN50I_DMIC_EN_CTL_CHAN_MASK,
+				   SUN50I_DMIC_EN_CTL_CHAN(host->chan_en));
+		/* Global enable */
+		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
+				   SUN50I_DMIC_EN_CTL_GLOBE,
+				   SUN50I_DMIC_EN_CTL_GLOBE);
+	} else {
+		/* DRQ DISABLE */
+		regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
+				   SUN50I_DMIC_FIFO_DRQ_EN, 0);
+		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
+				   SUN50I_DMIC_EN_CTL_CHAN_MASK,
+				   SUN50I_DMIC_EN_CTL_CHAN(0));
+		/* Global disable */
+		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
+				   SUN50I_DMIC_EN_CTL_GLOBE, 0);
+	}
+}
+
+static int sun50i_dmic_startup(struct snd_pcm_substream *substream,
+			       struct snd_soc_dai *cpu_dai)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(rtd->cpu_dai);
+
+	/* only support capture */
+	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
+		return -EINVAL;
+
+	regmap_write(host->regmap, SUN50I_DMIC_INT_STA,
+		     SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING |
+		     SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING);
+	regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+			   SUN50I_DMIC_RXFIFO_CTL_FLUSH,
+			   SUN50I_DMIC_RXFIFO_CTL_FLUSH);
+	regmap_write(host->regmap, SUN50I_DMIC_CNT, SUN50I_DMIC_CNT_N);
+
+	return 0;
+}
+
+static int sun50i_dmic_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *cpu_dai)
+{
+	int i = 0;
+	unsigned long rate = params_rate(params);
+	unsigned int channels = params_channels(params);
+	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
+	struct platform_device *pdev = host->pdev;
+	struct dmic_rate dmic_rate_s[] = {
+		{44100, 0x0},
+		{48000, 0x0},
+		{22050, 0x2},
+		{24000, 0x2},
+		{11025, 0x4},
+		{12000, 0x4},
+		{32000, 0x1},
+		{16000, 0x3},
+		{8000,  0x5},
+	};
+
+	/* DMIC num is N+1 */
+	regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM,
+			   SUN50I_DMIC_CH_NUM_N_MASK,
+			   SUN50I_DMIC_CH_NUM_N(channels - 1));
+	host->chan_en = (1 << channels) - 1;
+	regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en);
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+				   SUN50I_DMIC_RXFIFO_CTL_MODE,
+				   SUN50I_DMIC_RXFIFO_CTL_MODE);
+		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION, 0);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+				   SUN50I_DMIC_RXFIFO_CTL_MODE, 0);
+		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION,
+				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
+		break;
+	default:
+		dev_err(&pdev->dev, "Invalid format!\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(dmic_rate_s); i++) {
+		if (dmic_rate_s[i].samplerate == rate) {
+			regmap_update_bits(host->regmap, SUN50I_DMIC_SR,
+					   SUN50I_DMIC_SR_SAMOLE_RATE_MASK,
+					   SUN50I_DMIC_SR_SAMOLE_RATE(dmic_rate_s[i].rate_bit));
+			break;
+		}
+	}
+	if (i == ARRAY_SIZE(dmic_rate_s)) {
+		dev_err(&pdev->dev, "Invalid rate!\n");
+		return -EINVAL;
+	}
+
+	/* oversamplerate adjust */
+	if (params_rate(params) >= 24000)
+		regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
+				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE,
+				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE);
+	else
+		regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
+				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE, 0);
+
+	return 0;
+}
+
+static int sun50i_dmic_trigger(struct snd_pcm_substream *substream, int cmd,
+			       struct snd_soc_dai *dai)
+{
+	int ret = 0;
+	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
+
+	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
+		return -EINVAL;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		sun50i_snd_rxctrl_enable(substream, host, true);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		sun50i_snd_rxctrl_enable(substream, host, false);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int sun50i_dmic_set_sysclk(struct snd_soc_dai *dai, int clk_id,
+				 unsigned int freq, int dir)
+{
+	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
+
+	if (clk_set_rate(host->dmic_clk, freq)) {
+		dev_err(dai->dev, "Freq : %u not support\n", freq);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sun50i_dmic_soc_dai_probe(struct snd_soc_dai *dai)
+{
+	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
+
+	snd_soc_dai_init_dma_data(dai, NULL, &host->dma_params_rx);
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops sun50i_dmic_dai_ops = {
+	.startup	= sun50i_dmic_startup,
+	.trigger	= sun50i_dmic_trigger,
+	.hw_params	= sun50i_dmic_hw_params,
+	.set_sysclk	= sun50i_dmic_set_sysclk,
+};
+
+static const struct regmap_config sun50i_dmic_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = SUN50I_DMIC_VERSION,
+	.cache_type = REGCACHE_NONE,
+};
+
+#define	SUN50I_DMIC_RATES (SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_KNOT)
+#define SUN50I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
+
+static struct snd_soc_dai_driver sun50i_dmic_dai = {
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 8,
+		.rates = SUN50I_DMIC_RATES,
+		.formats = SUN50I_FORMATS,
+	},
+	.probe = sun50i_dmic_soc_dai_probe,
+	.ops = &sun50i_dmic_dai_ops,
+	.name = "dmic",
+};
+
+static const struct of_device_id sun50i_dmic_of_match[] = {
+	{
+		.compatible = "allwinner,sun50i-h6-dmic",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun50i_dmic_of_match);
+
+static const struct snd_soc_component_driver sun50i_dmic_component = {
+	.name		= "sun50i-dmic",
+};
+
+static int sun50i_dmic_runtime_suspend(struct device *dev)
+{
+	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(host->dmic_clk);
+	clk_disable_unprepare(host->apb_clk);
+
+	return 0;
+}
+
+static int sun50i_dmic_runtime_resume(struct device *dev)
+{
+	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(host->dmic_clk);
+	if (ret)
+		return ret;
+	ret = clk_prepare_enable(host->apb_clk);
+	if (ret)
+		clk_disable_unprepare(host->dmic_clk);
+
+	return ret;
+}
+
+static int sun50i_dmic_probe(struct platform_device *pdev)
+{
+	struct sun50i_dmic_dev *host;
+	struct resource *res;
+	int ret;
+	void __iomem *base;
+
+	host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	host->pdev = pdev;
+
+	/* Get the addresses */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(base),
+				     "get resource failed.\n");
+
+	host->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+						&sun50i_dmic_regmap_config);
+
+	/* Clocks */
+	host->apb_clk = devm_clk_get(&pdev->dev, "apb");
+	if (IS_ERR(host->apb_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(host->apb_clk),
+				     "failed to get apb clock.\n");
+
+	host->dmic_clk = devm_clk_get(&pdev->dev, "dmic");
+	if (IS_ERR(host->dmic_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk),
+				     "failed to get dmic clock.\n");
+
+	host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;
+	host->dma_params_rx.maxburst = 8;
+	host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+
+	platform_set_drvdata(pdev, host);
+
+	host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		dev_err(&pdev->dev, "Failed to get reset: %d\n", ret);
+		return ret;
+	}
+	if (!IS_ERR(host->rst))
+		reset_control_deassert(host->rst);
+
+	ret = devm_snd_soc_register_component(&pdev->dev,
+				&sun50i_dmic_component, &sun50i_dmic_dai, 1);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to register component.\n");
+
+	pm_runtime_enable(&pdev->dev);
+	if (!pm_runtime_enabled(&pdev->dev)) {
+		ret = sun50i_dmic_runtime_resume(&pdev->dev);
+		if (ret)
+			goto err_unregister;
+	}
+
+	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
+	if (ret)
+		goto err_suspend;
+
+	return 0;
+err_suspend:
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		sun50i_dmic_runtime_suspend(&pdev->dev);
+err_unregister:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
+}
+
+static int sun50i_dmic_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		sun50i_dmic_runtime_suspend(&pdev->dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sun50i_dmic_pm = {
+	SET_RUNTIME_PM_OPS(sun50i_dmic_runtime_suspend,
+			   sun50i_dmic_runtime_resume, NULL)
+};
+
+static struct platform_driver sun50i_dmic_driver = {
+	.driver		= {
+		.name	= "sun50i-dmic",
+		.of_match_table = of_match_ptr(sun50i_dmic_of_match),
+		.pm	= &sun50i_dmic_pm,
+	},
+	.probe		= sun50i_dmic_probe,
+	.remove		= sun50i_dmic_remove,
+};
+
+module_platform_driver(sun50i_dmic_driver);
+
+MODULE_DESCRIPTION("Allwinner sun50i DMIC SoC Interface");
+MODULE_AUTHOR("Ban Tao <fengzheng923@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:sun50i-dmic");
-- 
2.29.0

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Philipp Zabel]

Hi Ban,

On Tue, 2021-06-15 at 21:03 +0800, Ban Tao wrote:
> The Allwinner H6 and later SoCs have an DMIC block
> which is capable of capture.
> 
> Signed-off-by: Ban Tao <fengzheng923@gmail.com>
[...]
> diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
> new file mode 100644
> index 000000000000..78d512d93974
> --- /dev/null
> +++ b/sound/soc/sunxi/sun50i-dmic.c
> @@ -0,0 +1,408 @@
[...]
> +	host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		dev_err(&pdev->dev, "Failed to get reset: %d\n", ret);
> +		return ret;
> +	}

Please don't ignore errors. For example:

	if (IS_ERR(host->rst))
		return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
				     "Failed to  get reset\n");

devm_reset_control_get_optional_exclusive() will return NULL if no reset
is specified in the device tree.

> +	if (!IS_ERR(host->rst))
> +		reset_control_deassert(host->rst);

Then this is not necessary. Just use:

	reset_control_deassert(host->rst);

regards
Philipp

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Tue, Jun 15, 2021 at 09:03:26PM +0800, Ban Tao wrote:

Other than a few small things this looks good:

> +M:	Ban Tao <fengzheng923@gmail.com>
> +L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
> +F:	sound/soc/sunxi/sun50i-dmic.c

Not the binding document?

> @@ -0,0 +1,408 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ALSA SoC DMIC Audio Layer
> + *
> + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static void sun50i_snd_rxctrl_enable(struct snd_pcm_substream *substream,
> +				    struct sun50i_dmic_dev *host, bool enable)
> +{
> +	if (enable) {

> +	} else {

> +static int sun50i_dmic_trigger(struct snd_pcm_substream *substream, int cmd,
> +			       struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
> +
> +	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> +		return -EINVAL;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		sun50i_snd_rxctrl_enable(substream, host, true);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		sun50i_snd_rxctrl_enable(substream, host, false);
> +		break;

This is the only caller of _rxctrl_enable() and _rxctrl_enable() shares
no code between the two cases - just inline _rxctrl_enable() here, it's
clearer what's going on.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Samuel Holland]

Hello,

On 6/15/21 8:03 AM, Ban Tao wrote:
> The Allwinner H6 and later SoCs have an DMIC block
> which is capable of capture.
> 
> Signed-off-by: Ban Tao <fengzheng923@gmail.com>
> ---
>  MAINTAINERS                   |   7 +
>  sound/soc/sunxi/Kconfig       |   8 +
>  sound/soc/sunxi/Makefile      |   1 +
>  sound/soc/sunxi/sun50i-dmic.c | 408 ++++++++++++++++++++++++++++++++++
>  4 files changed, 424 insertions(+)
>  create mode 100644 sound/soc/sunxi/sun50i-dmic.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b4094f07214e..1b87225c39b0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -760,6 +760,13 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/staging/media/sunxi/cedrus/
>  
> +ALLWINNER DMIC DRIVERS
> +M:	Ban Tao <fengzheng923@gmail.com>
> +L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
> +F:	sound/soc/sunxi/sun50i-dmic.c
> +
>  ALPHA PORT
>  M:	Richard Henderson <rth@twiddle.net>
>  M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
> index ddcaaa98d3cb..2a3bf7722e11 100644
> --- a/sound/soc/sunxi/Kconfig
> +++ b/sound/soc/sunxi/Kconfig
> @@ -56,6 +56,14 @@ config SND_SUN4I_SPDIF
>  	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
>  	  A10 and affiliated SoCs.
>  
> +config SND_SUN50I_DMIC
> +	tristate "Allwinner H6 DMIC Support"
> +	depends on (OF && ARCH_SUNXI) || COMPILE_TEST
> +	select SND_SOC_GENERIC_DMAENGINE_PCM
> +	help
> +	  Say Y or M to add support for the DMIC audio block in the Allwinner
> +	  H6 and affiliated SoCs.
> +
>  config SND_SUN8I_ADDA_PR_REGMAP
>  	tristate
>  	select REGMAP
> diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
> index a86be340a076..4483fe9c94ef 100644
> --- a/sound/soc/sunxi/Makefile
> +++ b/sound/soc/sunxi/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
>  obj-$(CONFIG_SND_SUN50I_CODEC_ANALOG) += sun50i-codec-analog.o
>  obj-$(CONFIG_SND_SUN8I_CODEC) += sun8i-codec.o
>  obj-$(CONFIG_SND_SUN8I_ADDA_PR_REGMAP) += sun8i-adda-pr-regmap.o
> +obj-$(CONFIG_SND_SUN50I_DMIC) += sun50i-dmic.o
> diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
> new file mode 100644
> index 000000000000..78d512d93974
> --- /dev/null
> +++ b/sound/soc/sunxi/sun50i-dmic.c
> @@ -0,0 +1,408 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ALSA SoC DMIC Audio Layer
> + *
> + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +
> +
> +#define SUN50I_DMIC_EN_CTL		(0x00)
> +	#define SUN50I_DMIC_EN_CTL_GLOBE			BIT(8)
> +	#define SUN50I_DMIC_EN_CTL_CHAN(v)		((v) << 0)
> +	#define SUN50I_DMIC_EN_CTL_CHAN_MASK		GENMASK(7, 0)
> +#define SUN50I_DMIC_SR			(0x04)
> +	#define SUN50I_DMIC_SR_SAMOLE_RATE(v)		((v) << 0)
> +	#define SUN50I_DMIC_SR_SAMOLE_RATE_MASK		GENMASK(2, 0)
> +#define SUN50I_DMIC_CTL			(0x08)
> +	#define SUN50I_DMIC_CTL_OVERSAMPLE_RATE		BIT(0)
> +#define SUN50I_DMIC_DATA			(0x10)
> +#define SUN50I_DMIC_INTC			(0x14)
> +	#define SUN50I_DMIC_FIFO_DRQ_EN			BIT(2)
> +#define SUN50I_DMIC_INT_STA		(0x18)
> +	#define SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING	BIT(1)
> +	#define SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING	BIT(0)
> +#define SUN50I_DMIC_RXFIFO_CTL		(0x1c)
> +	#define SUN50I_DMIC_RXFIFO_CTL_FLUSH		BIT(31)
> +	#define SUN50I_DMIC_RXFIFO_CTL_MODE		BIT(9)
> +	#define SUN50I_DMIC_RXFIFO_CTL_RESOLUTION	BIT(8)
> +#define SUN50I_DMIC_CH_NUM		(0x24)
> +	#define SUN50I_DMIC_CH_NUM_N(v)			((v) << 0)
> +	#define SUN50I_DMIC_CH_NUM_N_MASK		GENMASK(2, 0)
> +#define SUN50I_DMIC_CNT			(0x2c)
> +	#define SUN50I_DMIC_CNT_N			BIT(0)
> +#define SUN50I_DMIC_HPF_CTRL		(0x38)
> +#define SUN50I_DMIC_VERSION		(0x50)
> +
> +
> +struct sun50i_dmic_dev {
> +	struct platform_device *pdev;
> +	struct clk *dmic_clk;
> +	struct clk *apb_clk;
> +	struct reset_control *rst;
> +	struct regmap *regmap;
> +	struct snd_dmaengine_dai_dma_data dma_params_rx;
> +	unsigned int chan_en;
> +};
> +
> +struct dmic_rate {
> +	unsigned int samplerate;
> +	unsigned int rate_bit;
> +};
> +
> +static void sun50i_snd_rxctrl_enable(struct snd_pcm_substream *substream,
> +				    struct sun50i_dmic_dev *host, bool enable)
> +{
> +	if (enable) {
> +		/* DRQ ENABLE */
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
> +				   SUN50I_DMIC_FIFO_DRQ_EN,
> +				   SUN50I_DMIC_FIFO_DRQ_EN);
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
> +				   SUN50I_DMIC_EN_CTL_CHAN_MASK,
> +				   SUN50I_DMIC_EN_CTL_CHAN(host->chan_en));
> +		/* Global enable */
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
> +				   SUN50I_DMIC_EN_CTL_GLOBE,
> +				   SUN50I_DMIC_EN_CTL_GLOBE);
> +	} else {
> +		/* DRQ DISABLE */
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
> +				   SUN50I_DMIC_FIFO_DRQ_EN, 0);
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
> +				   SUN50I_DMIC_EN_CTL_CHAN_MASK,
> +				   SUN50I_DMIC_EN_CTL_CHAN(0));
> +		/* Global disable */
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
> +				   SUN50I_DMIC_EN_CTL_GLOBE, 0);
> +	}
> +}
> +
> +static int sun50i_dmic_startup(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *cpu_dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(rtd->cpu_dai);

This is equivalent to the simpler code you use below:

struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);

> +
> +	/* only support capture */
> +	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> +		return -EINVAL;
> +
> +	regmap_write(host->regmap, SUN50I_DMIC_INT_STA,
> +		     SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING |
> +		     SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING);

The interrupt is never enabled, so do we really care about IRQ status?

> +	regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> +			   SUN50I_DMIC_RXFIFO_CTL_FLUSH,
> +			   SUN50I_DMIC_RXFIFO_CTL_FLUSH);
> +	regmap_write(host->regmap, SUN50I_DMIC_CNT, SUN50I_DMIC_CNT_N);
> +
> +	return 0;
> +}
> +
> +static int sun50i_dmic_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *cpu_dai)
> +{
> +	int i = 0;
> +	unsigned long rate = params_rate(params);
> +	unsigned int channels = params_channels(params);
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct platform_device *pdev = host->pdev;
> +	struct dmic_rate dmic_rate_s[] = {
> +		{44100, 0x0},
> +		{48000, 0x0},
> +		{22050, 0x2},
> +		{24000, 0x2},
> +		{11025, 0x4},
> +		{12000, 0x4},
> +		{32000, 0x1},
> +		{16000, 0x3},
> +		{8000,  0x5},
> +	};
> +
> +	/* DMIC num is N+1 */
> +	regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM,
> +			   SUN50I_DMIC_CH_NUM_N_MASK,
> +			   SUN50I_DMIC_CH_NUM_N(channels - 1));
> +	host->chan_en = (1 << channels) - 1;
> +	regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en);
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> +				   SUN50I_DMIC_RXFIFO_CTL_MODE,
> +				   SUN50I_DMIC_RXFIFO_CTL_MODE);
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> +				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION, 0);
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> +				   SUN50I_DMIC_RXFIFO_CTL_MODE, 0);

In SNDRV_PCM_FORMAT_S24_LE, the sample is in the three least significant
bytes, so wouldn't we want to still use mode 1 here? To me, mode 0 only
makes sense when using SNDRV_PCM_FORMAT_S32_LE.

> +		regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> +				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION,
> +				   SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "Invalid format!\n");

The calls to dev_err() can use cpu_dai->dev. Then you don't need to keep
a pointer to pdev in the driver data.

> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(dmic_rate_s); i++) {
> +		if (dmic_rate_s[i].samplerate == rate) {
> +			regmap_update_bits(host->regmap, SUN50I_DMIC_SR,
> +					   SUN50I_DMIC_SR_SAMOLE_RATE_MASK,
> +					   SUN50I_DMIC_SR_SAMOLE_RATE(dmic_rate_s[i].rate_bit));
> +			break;
> +		}
> +	}
> +	if (i == ARRAY_SIZE(dmic_rate_s)) {
> +		dev_err(&pdev->dev, "Invalid rate!\n");
> +		return -EINVAL;
> +	}
> +
> +	/* oversamplerate adjust */
> +	if (params_rate(params) >= 24000)
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
> +				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE,
> +				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE);
> +	else
> +		regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
> +				   SUN50I_DMIC_CTL_OVERSAMPLE_RATE, 0);
> +
> +	return 0;
> +}
> +
> +static int sun50i_dmic_trigger(struct snd_pcm_substream *substream, int cmd,
> +			       struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
> +
> +	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> +		return -EINVAL;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		sun50i_snd_rxctrl_enable(substream, host, true);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		sun50i_snd_rxctrl_enable(substream, host, false);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int sun50i_dmic_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> +				 unsigned int freq, int dir)
> +{
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
> +
> +	if (clk_set_rate(host->dmic_clk, freq)) {

Currently, mainline does not have any sunxi-specific machine drivers. We
are using simple-audio-card for sun50i. And asoc_simple_hw_params() only
calls .set_sysclk if mclk-fs is set to some fixed value, from the device
tree. However, for sunxi, mclk-fs cannot be a fixed value, because
sysclk is the same 22/24 MHz regardless of sample rate. So .set_sysclk
will never be called.

So for now I would suggest calling clk_set_rate() from .hw_params, using
something like sun8i_codec_get_sysclk_rate() to get the right frequency.

> +		dev_err(dai->dev, "Freq : %u not support\n", freq);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sun50i_dmic_soc_dai_probe(struct snd_soc_dai *dai)
> +{
> +	struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
> +
> +	snd_soc_dai_init_dma_data(dai, NULL, &host->dma_params_rx);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops sun50i_dmic_dai_ops = {
> +	.startup	= sun50i_dmic_startup,
> +	.trigger	= sun50i_dmic_trigger,
> +	.hw_params	= sun50i_dmic_hw_params,
> +	.set_sysclk	= sun50i_dmic_set_sysclk,
> +};
> +
> +static const struct regmap_config sun50i_dmic_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = SUN50I_DMIC_VERSION,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +#define	SUN50I_DMIC_RATES (SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_KNOT)

If you use SNDRV_PCM_RATE_KNOT, you also need to provide a list of
sample rates as a constraint in .startup.

> +#define SUN50I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
> +
> +static struct snd_soc_dai_driver sun50i_dmic_dai = {
> +	.capture = {
> +		.channels_min = 1,
> +		.channels_max = 8,
> +		.rates = SUN50I_DMIC_RATES,
> +		.formats = SUN50I_FORMATS,
> +	},
> +	.probe = sun50i_dmic_soc_dai_probe,
> +	.ops = &sun50i_dmic_dai_ops,
> +	.name = "dmic",
> +};
> +
> +static const struct of_device_id sun50i_dmic_of_match[] = {
> +	{
> +		.compatible = "allwinner,sun50i-h6-dmic",
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun50i_dmic_of_match);
> +
> +static const struct snd_soc_component_driver sun50i_dmic_component = {
> +	.name		= "sun50i-dmic",
> +};
> +
> +static int sun50i_dmic_runtime_suspend(struct device *dev)
> +{
> +	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(host->dmic_clk);
> +	clk_disable_unprepare(host->apb_clk);
> +
> +	return 0;
> +}
> +
> +static int sun50i_dmic_runtime_resume(struct device *dev)
> +{
> +	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(host->dmic_clk);
> +	if (ret)
> +		return ret;
> +	ret = clk_prepare_enable(host->apb_clk);
> +	if (ret)
> +		clk_disable_unprepare(host->dmic_clk);
> +
> +	return ret;
> +}
> +
> +static int sun50i_dmic_probe(struct platform_device *pdev)
> +{
> +	struct sun50i_dmic_dev *host;
> +	struct resource *res;
> +	int ret;
> +	void __iomem *base;
> +
> +	host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	host->pdev = pdev;
> +
> +	/* Get the addresses */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
> +				     "get resource failed.\n");
> +
> +	host->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +						&sun50i_dmic_regmap_config);
> +
> +	/* Clocks */
> +	host->apb_clk = devm_clk_get(&pdev->dev, "apb");
> +	if (IS_ERR(host->apb_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(host->apb_clk),
> +				     "failed to get apb clock.\n");
> +
> +	host->dmic_clk = devm_clk_get(&pdev->dev, "dmic");
> +	if (IS_ERR(host->dmic_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk),
> +				     "failed to get dmic clock.\n");
> +
> +	host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;
> +	host->dma_params_rx.maxburst = 8;
> +	host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;

This needs to be DMA_SLAVE_BUSWIDTH_4_BYTES if you are using larger than
16-bit samples. See for example sun4i_i2s_hw_params().

Regards,
Samuel

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[班涛]

Hi,

Mark Brown <broonie@kernel.org> 于2021年6月15日周二 下午9:22写道：
>
> On Tue, Jun 15, 2021 at 09:03:26PM +0800, Ban Tao wrote:
>
> Other than a few small things this looks good:
>
> > +M:   Ban Tao <fengzheng923@gmail.com>
> > +L:   alsa-devel@alsa-project.org (moderated for non-subscribers)
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
> > +F:   sound/soc/sunxi/sun50i-dmic.c
>
> Not the binding document?
>
> > @@ -0,0 +1,408 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * ALSA SoC DMIC Audio Layer
> > + *
> > + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.
>
For example;
// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * This driver supports the DMIC in Allwinner's H6 SoCs.
 *
 * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
 *
 */
is this OK?


> > +static void sun50i_snd_rxctrl_enable(struct snd_pcm_substream *substream,
> > +                                 struct sun50i_dmic_dev *host, bool enable)
> > +{
> > +     if (enable) {
>
> > +     } else {
>
> > +static int sun50i_dmic_trigger(struct snd_pcm_substream *substream, int cmd,
> > +                            struct snd_soc_dai *dai)
> > +{
> > +     int ret = 0;
> > +     struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
> > +
> > +     if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> > +             return -EINVAL;
> > +
> > +     switch (cmd) {
> > +     case SNDRV_PCM_TRIGGER_START:
> > +     case SNDRV_PCM_TRIGGER_RESUME:
> > +     case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +             sun50i_snd_rxctrl_enable(substream, host, true);
> > +             break;
> > +
> > +     case SNDRV_PCM_TRIGGER_STOP:
> > +     case SNDRV_PCM_TRIGGER_SUSPEND:
> > +     case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > +             sun50i_snd_rxctrl_enable(substream, host, false);
> > +             break;
>
> This is the only caller of _rxctrl_enable() and _rxctrl_enable() shares
> no code between the two cases - just inline _rxctrl_enable() here, it's
> clearer what's going on.
>
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(&pdev->dev, res);
>
> devm_platform_ioremap_resource()

But I need to get the register base address of DMIC. E.g res->start.
host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thu, Jun 17, 2021 at 03:42:43PM +0800, 班涛 wrote:
> Mark Brown <broonie@kernel.org> 于2021年6月15日周二 下午9:22写道：

> > > @@ -0,0 +1,408 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * ALSA SoC DMIC Audio Layer
> > > + *
> > > + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> > > + *

> > Please make the entire comment a C++ one so things look more
> > intentional.

> For example;
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
>  * This driver supports the DMIC in Allwinner's H6 SoCs.
>  *
>  * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
>  *
>  */
> is this OK?

No, that's what you have already make the entire thing a C++ comment
with //s.

> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     base = devm_ioremap_resource(&pdev->dev, res);
> >
> > devm_platform_ioremap_resource()
> 
> But I need to get the register base address of DMIC. E.g res->start.
> host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;

OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[班涛]

Hi，

Mark Brown <broonie@kernel.org> 于2021年6月17日周四 下午6:48写道：
>
> On Thu, Jun 17, 2021 at 03:42:43PM +0800, 班涛 wrote:
> > Mark Brown <broonie@kernel.org> 于2021年6月15日周二 下午9:22写道：
>
> > > > @@ -0,0 +1,408 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * ALSA SoC DMIC Audio Layer
> > > > + *
> > > > + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> > > > + *
>
> > > Please make the entire comment a C++ one so things look more
> > > intentional.
>
> > For example;
> > // SPDX-License-Identifier: GPL-2.0-or-later
> > /*
> >  * This driver supports the DMIC in Allwinner's H6 SoCs.
> >  *
> >  * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
> >  *
> >  */
> > is this OK?
>
> No, that's what you have already make the entire thing a C++ comment
> with //s.

I don’t understand. For example, sun4i-codec.c sun4i-i2s.c
sun8i-codec.c and sun4i-spdif.c files are the same as mine.
Which file can I refer to? what should I do......
>
> > > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +     base = devm_ioremap_resource(&pdev->dev, res);
> > >
> > > devm_platform_ioremap_resource()
> >
> > But I need to get the register base address of DMIC. E.g res->start.
> > host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;
>
> OK.

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Thu, Jun 17, 2021 at 07:50:42PM +0800, 班涛 wrote:
> Mark Brown <broonie@kernel.org> 于2021年6月17日周四 下午6:48写道：
> > On Thu, Jun 17, 2021 at 03:42:43PM +0800, 班涛 wrote:
> > > Mark Brown <broonie@kernel.org> 于2021年6月15日周二 下午9:22写道：

> > No, that's what you have already make the entire thing a C++ comment
> > with //s.

> I don’t understand. For example, sun4i-codec.c sun4i-i2s.c
> sun8i-codec.c and sun4i-spdif.c files are the same as mine.

Other people doing a bad job is no excuse for doing a bad job yourself.

> Which file can I refer to? what should I do......

Make every line of the comment start with //.  See soc-core.c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[班涛]

Hi,

Mark Brown <broonie@kernel.org> 于2021年6月17日周四 下午8:08写道：
>
> On Thu, Jun 17, 2021 at 07:50:42PM +0800, 班涛 wrote:
> > Mark Brown <broonie@kernel.org> 于2021年6月17日周四 下午6:48写道：
> > > On Thu, Jun 17, 2021 at 03:42:43PM +0800, 班涛 wrote:
> > > > Mark Brown <broonie@kernel.org> 于2021年6月15日周二 下午9:22写道：
>
> > > No, that's what you have already make the entire thing a C++ comment
> > > with //s.
>
> > I don’t understand. For example, sun4i-codec.c sun4i-i2s.c
> > sun8i-codec.c and sun4i-spdif.c files are the same as mine.
>
> Other people doing a bad job is no excuse for doing a bad job yourself.
>
> > Which file can I refer to? what should I do......
>
> Make every line of the comment start with //.  See soc-core.c

Thanks, I understand, I will fix it in v2.

Re: [PATCH 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

[Samuel Holland]

On 6/16/21 1:21 AM, 班涛 wrote:
> Hi，
> 
> Samuel Holland <samuel@sholland.org> 于2021年6月16日周三 上午11:06写道：
> 
>> Hello,
>>
>> On 6/15/21 8:03 AM, Ban Tao wrote:
>>> The Allwinner H6 and later SoCs have an DMIC block
>>> which is capable of capture.
>>>
>>> Signed-off-by: Ban Tao <fengzheng923@gmail.com>
>>> ---
>>>  MAINTAINERS                   |   7 +
>>>  sound/soc/sunxi/Kconfig       |   8 +
>>>  sound/soc/sunxi/Makefile      |   1 +
>>>  sound/soc/sunxi/sun50i-dmic.c | 408 ++++++++++++++++++++++++++++++++++
>>>  4 files changed, 424 insertions(+)
>>>  create mode 100644 sound/soc/sunxi/sun50i-dmic.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index b4094f07214e..1b87225c39b0 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -760,6 +760,13 @@ L:       linux-media@vger.kernel.org
>>>  S:   Maintained
>>>  F:   drivers/staging/media/sunxi/cedrus/
>>>
>>> +ALLWINNER DMIC DRIVERS
>>> +M:   Ban Tao <fengzheng923@gmail.com>
>>> +L:   alsa-devel@alsa-project.org (moderated for non-subscribers)
>>> +S:   Maintained
>>> +F:
>>  Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
>>> +F:   sound/soc/sunxi/sun50i-dmic.c
>>> +
>>>  ALPHA PORT
>>>  M:   Richard Henderson <rth@twiddle.net>
>>>  M:   Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>>> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
>>> index ddcaaa98d3cb..2a3bf7722e11 100644
>>> --- a/sound/soc/sunxi/Kconfig
>>> +++ b/sound/soc/sunxi/Kconfig
>>> @@ -56,6 +56,14 @@ config SND_SUN4I_SPDIF
>>>         Say Y or M to add support for the S/PDIF audio block in the
>> Allwinner
>>>         A10 and affiliated SoCs.
>>>
>>> +config SND_SUN50I_DMIC
>>> +     tristate "Allwinner H6 DMIC Support"
>>> +     depends on (OF && ARCH_SUNXI) || COMPILE_TEST
>>> +     select SND_SOC_GENERIC_DMAENGINE_PCM
>>> +     help
>>> +       Say Y or M to add support for the DMIC audio block in the
>> Allwinner
>>> +       H6 and affiliated SoCs.
>>> +
>>>  config SND_SUN8I_ADDA_PR_REGMAP
>>>       tristate
>>>       select REGMAP
>>> diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
>>> index a86be340a076..4483fe9c94ef 100644
>>> --- a/sound/soc/sunxi/Makefile
>>> +++ b/sound/soc/sunxi/Makefile
>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) +=
>> sun8i-codec-analog.o
>>>  obj-$(CONFIG_SND_SUN50I_CODEC_ANALOG) += sun50i-codec-analog.o
>>>  obj-$(CONFIG_SND_SUN8I_CODEC) += sun8i-codec.o
>>>  obj-$(CONFIG_SND_SUN8I_ADDA_PR_REGMAP) += sun8i-adda-pr-regmap.o
>>> +obj-$(CONFIG_SND_SUN50I_DMIC) += sun50i-dmic.o
>>> diff --git a/sound/soc/sunxi/sun50i-dmic.c
>> b/sound/soc/sunxi/sun50i-dmic.c
>>> new file mode 100644
>>> index 000000000000..78d512d93974
>>> --- /dev/null
>>> +++ b/sound/soc/sunxi/sun50i-dmic.c
>>> @@ -0,0 +1,408 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * ALSA SoC DMIC Audio Layer
>>> + *
>>> + * Copyright 2021 Ban Tao <fengzheng923@gmail.com>
>>> + *
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/device.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/reset.h>
>>> +#include <sound/dmaengine_pcm.h>
>>> +#include <sound/pcm_params.h>
>>> +#include <sound/soc.h>
>>> +
>>> +
>>> +#define SUN50I_DMIC_EN_CTL           (0x00)
>>> +     #define SUN50I_DMIC_EN_CTL_GLOBE                        BIT(8)
>>> +     #define SUN50I_DMIC_EN_CTL_CHAN(v)              ((v) << 0)
>>> +     #define SUN50I_DMIC_EN_CTL_CHAN_MASK            GENMASK(7, 0)
>>> +#define SUN50I_DMIC_SR                       (0x04)
>>> +     #define SUN50I_DMIC_SR_SAMOLE_RATE(v)           ((v) << 0)
>>> +     #define SUN50I_DMIC_SR_SAMOLE_RATE_MASK         GENMASK(2, 0)
>>> +#define SUN50I_DMIC_CTL                      (0x08)
>>> +     #define SUN50I_DMIC_CTL_OVERSAMPLE_RATE         BIT(0)
>>> +#define SUN50I_DMIC_DATA                     (0x10)
>>> +#define SUN50I_DMIC_INTC                     (0x14)
>>> +     #define SUN50I_DMIC_FIFO_DRQ_EN                 BIT(2)
>>> +#define SUN50I_DMIC_INT_STA          (0x18)
>>> +     #define SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING BIT(1)
>>> +     #define SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING    BIT(0)
>>> +#define SUN50I_DMIC_RXFIFO_CTL               (0x1c)
>>> +     #define SUN50I_DMIC_RXFIFO_CTL_FLUSH            BIT(31)
>>> +     #define SUN50I_DMIC_RXFIFO_CTL_MODE             BIT(9)
>>> +     #define SUN50I_DMIC_RXFIFO_CTL_RESOLUTION       BIT(8)
>>> +#define SUN50I_DMIC_CH_NUM           (0x24)
>>> +     #define SUN50I_DMIC_CH_NUM_N(v)                 ((v) << 0)
>>> +     #define SUN50I_DMIC_CH_NUM_N_MASK               GENMASK(2, 0)
>>> +#define SUN50I_DMIC_CNT                      (0x2c)
>>> +     #define SUN50I_DMIC_CNT_N                       BIT(0)
>>> +#define SUN50I_DMIC_HPF_CTRL         (0x38)
>>> +#define SUN50I_DMIC_VERSION          (0x50)
>>> +
>>> +
>>> +struct sun50i_dmic_dev {
>>> +     struct platform_device *pdev;
>>> +     struct clk *dmic_clk;
>>> +     struct clk *apb_clk;
>>> +     struct reset_control *rst;
>>> +     struct regmap *regmap;
>>> +     struct snd_dmaengine_dai_dma_data dma_params_rx;
>>> +     unsigned int chan_en;
>>> +};
>>> +
>>> +struct dmic_rate {
>>> +     unsigned int samplerate;
>>> +     unsigned int rate_bit;
>>> +};
>>> +
>>> +static void sun50i_snd_rxctrl_enable(struct snd_pcm_substream
>> *substream,
>>> +                                 struct sun50i_dmic_dev *host, bool
>> enable)
>>> +{
>>> +     if (enable) {
>>> +             /* DRQ ENABLE */
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
>>> +                                SUN50I_DMIC_FIFO_DRQ_EN,
>>> +                                SUN50I_DMIC_FIFO_DRQ_EN);
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
>>> +                                SUN50I_DMIC_EN_CTL_CHAN_MASK,
>>> +                                SUN50I_DMIC_EN_CTL_CHAN(host->chan_en));
>>> +             /* Global enable */
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
>>> +                                SUN50I_DMIC_EN_CTL_GLOBE,
>>> +                                SUN50I_DMIC_EN_CTL_GLOBE);
>>> +     } else {
>>> +             /* DRQ DISABLE */
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_INTC,
>>> +                                SUN50I_DMIC_FIFO_DRQ_EN, 0);
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
>>> +                                SUN50I_DMIC_EN_CTL_CHAN_MASK,
>>> +                                SUN50I_DMIC_EN_CTL_CHAN(0));
>>> +             /* Global disable */
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_EN_CTL,
>>> +                                SUN50I_DMIC_EN_CTL_GLOBE, 0);
>>> +     }
>>> +}
>>> +
>>> +static int sun50i_dmic_startup(struct snd_pcm_substream *substream,
>>> +                            struct snd_soc_dai *cpu_dai)
>>> +{
>>> +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>> +     struct sun50i_dmic_dev *host =
>> snd_soc_dai_get_drvdata(rtd->cpu_dai);
>>
>> This is equivalent to the simpler code you use below:
>>
>> struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>>
>>> +
>>> +     /* only support capture */
>>> +     if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
>>> +             return -EINVAL;
>>> +
>>> +     regmap_write(host->regmap, SUN50I_DMIC_INT_STA,
>>> +                  SUN50I_DMIC_INT_STA_OVERRUN_IRQ_PENDING |
>>> +                  SUN50I_DMIC_INT_STA_DATA_IRQ_PENDING);
>>
>> The interrupt is never enabled, so do we really care about IRQ status?
>>
> 
> Ok, I will delete it .
> 
>>
>>> +     regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
>>> +                        SUN50I_DMIC_RXFIFO_CTL_FLUSH,
>>> +                        SUN50I_DMIC_RXFIFO_CTL_FLUSH);
>>> +     regmap_write(host->regmap, SUN50I_DMIC_CNT, SUN50I_DMIC_CNT_N);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int sun50i_dmic_hw_params(struct snd_pcm_substream *substream,
>>> +                              struct snd_pcm_hw_params *params,
>>> +                              struct snd_soc_dai *cpu_dai)
>>> +{
>>> +     int i = 0;
>>> +     unsigned long rate = params_rate(params);
>>> +     unsigned int channels = params_channels(params);
>>> +     struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>>> +     struct platform_device *pdev = host->pdev;
>>> +     struct dmic_rate dmic_rate_s[] = {
>>> +             {44100, 0x0},
>>> +             {48000, 0x0},
>>> +             {22050, 0x2},
>>> +             {24000, 0x2},
>>> +             {11025, 0x4},
>>> +             {12000, 0x4},
>>> +             {32000, 0x1},
>>> +             {16000, 0x3},
>>> +             {8000,  0x5},
>>> +     };
>>> +
>>> +     /* DMIC num is N+1 */
>>> +     regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM,
>>> +                        SUN50I_DMIC_CH_NUM_N_MASK,
>>> +                        SUN50I_DMIC_CH_NUM_N(channels - 1));
>>> +     host->chan_en = (1 << channels) - 1;
>>> +     regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en);
>>> +
>>> +     switch (params_format(params)) {
>>> +     case SNDRV_PCM_FORMAT_S16_LE:
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_MODE,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_MODE);
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_RESOLUTION, 0);
>>> +             break;
>>> +     case SNDRV_PCM_FORMAT_S24_LE:
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_MODE, 0);
>>
>> In SNDRV_PCM_FORMAT_S24_LE, the sample is in the three least significant
>> bytes, so wouldn't we want to still use mode 1 here? To me, mode 0 only
>> makes sense when using SNDRV_PCM_FORMAT_S32_LE.
>>
> 
> I don't understand. The DMIC does not support 32bits, why use
> SNDRV_PCM_FORMAT_S32_LE ?

The hardware supports extending the samples to 32 bits. So if userspace
wants 32-bit samples (for whatever reason), it is more efficient to do
the extension in hardware than in software. You do not need to add
support for SNDRV_PCM_FORMAT_S32_LE; the driver will work fine without it.

But this is unrelated to the fact that mode 0 is wrong for
SNDRV_PCM_FORMAT_S24_LE.

SNDRV_PCM_FORMAT_S16_LE => samples must be in 2 lower bytes.
   Mode 1 does this. The upper 2 bytes are ignored.

SNDRV_PCM_FORMAT_S24_LE => samples must be in 3 lower bytes.
   Mode 1 does this. The 21-bit sample is extended with 3 zeroes.

In fact, since only 21 bits in the 24-bit sample are meaningful, you
should set the sig_bits field in the DAI driver so userspace can know.

(Does it make sense that extending 21 -> 24 bits to fit in a standard
PCM format serves the same purpose as extending 21 -> 32 bits?)

>>
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_RESOLUTION,
>>> +                                SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
>>> +             break;
>>> +     default:
>>> +             dev_err(&pdev->dev, "Invalid format!\n");
>>
>> The calls to dev_err() can use cpu_dai->dev. Then you don't need to keep
>> a pointer to pdev in the driver data.
>>
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(dmic_rate_s); i++) {
>>> +             if (dmic_rate_s[i].samplerate == rate) {
>>> +                     regmap_update_bits(host->regmap, SUN50I_DMIC_SR,
>>> +                                        SUN50I_DMIC_SR_SAMOLE_RATE_MASK,
>>> +
>> SUN50I_DMIC_SR_SAMOLE_RATE(dmic_rate_s[i].rate_bit));
>>> +                     break;
>>> +             }
>>> +     }
>>> +     if (i == ARRAY_SIZE(dmic_rate_s)) {
>>> +             dev_err(&pdev->dev, "Invalid rate!\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     /* oversamplerate adjust */
>>> +     if (params_rate(params) >= 24000)
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
>>> +                                SUN50I_DMIC_CTL_OVERSAMPLE_RATE,
>>> +                                SUN50I_DMIC_CTL_OVERSAMPLE_RATE);
>>> +     else
>>> +             regmap_update_bits(host->regmap, SUN50I_DMIC_CTL,
>>> +                                SUN50I_DMIC_CTL_OVERSAMPLE_RATE, 0);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int sun50i_dmic_trigger(struct snd_pcm_substream *substream, int
>> cmd,
>>> +                            struct snd_soc_dai *dai)
>>> +{
>>> +     int ret = 0;
>>> +     struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
>>> +
>>> +     if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
>>> +             return -EINVAL;
>>> +
>>> +     switch (cmd) {
>>> +     case SNDRV_PCM_TRIGGER_START:
>>> +     case SNDRV_PCM_TRIGGER_RESUME:
>>> +     case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>>> +             sun50i_snd_rxctrl_enable(substream, host, true);
>>> +             break;
>>> +
>>> +     case SNDRV_PCM_TRIGGER_STOP:
>>> +     case SNDRV_PCM_TRIGGER_SUSPEND:
>>> +     case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>>> +             sun50i_snd_rxctrl_enable(substream, host, false);
>>> +             break;
>>> +
>>> +     default:
>>> +             ret = -EINVAL;
>>> +             break;
>>> +     }
>>> +     return ret;
>>> +}
>>> +
>>> +static int sun50i_dmic_set_sysclk(struct snd_soc_dai *dai, int clk_id,
>>> +                              unsigned int freq, int dir)
>>> +{
>>> +     struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
>>> +
>>> +     if (clk_set_rate(host->dmic_clk, freq)) {
>>
>> Currently, mainline does not have any sunxi-specific machine drivers. We
>> are using simple-audio-card for sun50i. And asoc_simple_hw_params() only
>> calls .set_sysclk if mclk-fs is set to some fixed value, from the device
>> tree. However, for sunxi, mclk-fs cannot be a fixed value, because
>> sysclk is the same 22/24 MHz regardless of sample rate. So .set_sysclk
>> will never be called.
>>
>> So for now I would suggest calling clk_set_rate() from .hw_params, using
>> something like sun8i_codec_get_sysclk_rate() to get the right frequency.
>>
> 
> Thanks, i will do it
> 
>>
>>> +             dev_err(dai->dev, "Freq : %u not support\n", freq);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int sun50i_dmic_soc_dai_probe(struct snd_soc_dai *dai)
>>> +{
>>> +     struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(dai);
>>> +
>>> +     snd_soc_dai_init_dma_data(dai, NULL, &host->dma_params_rx);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct snd_soc_dai_ops sun50i_dmic_dai_ops = {
>>> +     .startup        = sun50i_dmic_startup,
>>> +     .trigger        = sun50i_dmic_trigger,
>>> +     .hw_params      = sun50i_dmic_hw_params,
>>> +     .set_sysclk     = sun50i_dmic_set_sysclk,
>>> +};
>>> +
>>> +static const struct regmap_config sun50i_dmic_regmap_config = {
>>> +     .reg_bits = 32,
>>> +     .reg_stride = 4,
>>> +     .val_bits = 32,
>>> +     .max_register = SUN50I_DMIC_VERSION,
>>> +     .cache_type = REGCACHE_NONE,
>>> +};
>>> +
>>> +#define      SUN50I_DMIC_RATES (SNDRV_PCM_RATE_8000_48000 |
>> SNDRV_PCM_RATE_KNOT)
>>
>> If you use SNDRV_PCM_RATE_KNOT, you also need to provide a list of
>> sample rates as a constraint in .startup.
>>
>>> +#define SUN50I_FORMATS       (SNDRV_PCM_FMTBIT_S16_LE |
>> SNDRV_PCM_FMTBIT_S24_LE)
>>> +
>>> +static struct snd_soc_dai_driver sun50i_dmic_dai = {
>>> +     .capture = {
>>> +             .channels_min = 1,
>>> +             .channels_max = 8,
>>> +             .rates = SUN50I_DMIC_RATES,
>>> +             .formats = SUN50I_FORMATS,

".sig_bits = 21," should go here.

Regards,
Samuel

>>> +     },
>>> +     .probe = sun50i_dmic_soc_dai_probe,
>>> +     .ops = &sun50i_dmic_dai_ops,
>>> +     .name = "dmic",
>>> +};
>>> +
>>> +static const struct of_device_id sun50i_dmic_of_match[] = {
>>> +     {
>>> +             .compatible = "allwinner,sun50i-h6-dmic",
>>> +     },
>>> +     { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sun50i_dmic_of_match);
>>> +
>>> +static const struct snd_soc_component_driver sun50i_dmic_component = {
>>> +     .name           = "sun50i-dmic",
>>> +};
>>> +
>>> +static int sun50i_dmic_runtime_suspend(struct device *dev)
>>> +{
>>> +     struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
>>> +
>>> +     clk_disable_unprepare(host->dmic_clk);
>>> +     clk_disable_unprepare(host->apb_clk);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int sun50i_dmic_runtime_resume(struct device *dev)
>>> +{
>>> +     struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
>>> +     int ret;
>>> +
>>> +     ret = clk_prepare_enable(host->dmic_clk);
>>> +     if (ret)
>>> +             return ret;
>>> +     ret = clk_prepare_enable(host->apb_clk);
>>> +     if (ret)
>>> +             clk_disable_unprepare(host->dmic_clk);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int sun50i_dmic_probe(struct platform_device *pdev)
>>> +{
>>> +     struct sun50i_dmic_dev *host;
>>> +     struct resource *res;
>>> +     int ret;
>>> +     void __iomem *base;
>>> +
>>> +     host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
>>> +     if (!host)
>>> +             return -ENOMEM;
>>> +
>>> +     host->pdev = pdev;
>>> +
>>> +     /* Get the addresses */
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +     base = devm_ioremap_resource(&pdev->dev, res);
>>> +     if (IS_ERR(base))
>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(base),
>>> +                                  "get resource failed.\n");
>>> +
>>> +     host->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>> +
>>  &sun50i_dmic_regmap_config);
>>> +
>>> +     /* Clocks */
>>> +     host->apb_clk = devm_clk_get(&pdev->dev, "apb");
>>> +     if (IS_ERR(host->apb_clk))
>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(host->apb_clk),
>>> +                                  "failed to get apb clock.\n");
>>> +
>>> +     host->dmic_clk = devm_clk_get(&pdev->dev, "dmic");
>>> +     if (IS_ERR(host->dmic_clk))
>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk),
>>> +                                  "failed to get dmic clock.\n");
>>> +
>>> +     host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;
>>> +     host->dma_params_rx.maxburst = 8;
>>> +     host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>
>> This needs to be DMA_SLAVE_BUSWIDTH_4_BYTES if you are using larger than
>> 16-bit samples. See for example sun4i_i2s_hw_params().
>>
>> Regards,
>> Samuel
>>
>