* [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
@ 2021-07-11 12:20 fengzheng923
2021-07-15 7:47 ` Maxime Ripard
0 siblings, 1 reply; 4+ messages in thread
From: fengzheng923 @ 2021-07-11 12:20 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai, mripard, wens, jernej.skrabec,
fengzheng923, p.zabel, samuel, krzk
Cc: linux-kernel, alsa-devel, linux-arm-kernel, linux-sunxi
From: Ban Tao <fengzheng923@gmail.com>
The Allwinner H6 and later SoCs have an DMIC block
which is capable of capture.
Signed-off-by: Ban Tao <fengzheng923@gmail.com>
---
v1->v2:
1.Fix some compilation errors.
2.Modify some code styles.
---
v2->v3:
None.
---
v3->v4:
1.add sig_bits.
---
v4->v5:
None.
---
v5->v6:
1.Modify RXFIFO_CTL_MODE to mode 1.
---
MAINTAINERS | 7 +
sound/soc/sunxi/Kconfig | 8 +
sound/soc/sunxi/Makefile | 1 +
sound/soc/sunxi/sun50i-dmic.c | 403 ++++++++++++++++++++++++++++++++++
4 files changed, 419 insertions(+)
create mode 100644 sound/soc/sunxi/sun50i-dmic.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e924f9e5df97..8d700baaa3ca 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..bbac836ba4de
--- /dev/null
+++ b/sound/soc/sunxi/sun50i-dmic.c
@@ -0,0 +1,403 @@
+// 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>
+
+#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_SAMPLE_RATE(v) ((v) << 0)
+ #define SUN50I_DMIC_SR_SAMPLE_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 clk *dmic_clk;
+ struct clk *bus_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 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(asoc_rtd_to_cpu(rtd, 0));
+
+ /* only support capture */
+ if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
+ return -EINVAL;
+
+ 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 mclk = 0;
+ unsigned int channels = params_channels(params);
+ struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
+ static 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_RESOLUTION, 0);
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+ SUN50I_DMIC_RXFIFO_CTL_RESOLUTION,
+ SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
+ break;
+ default:
+ dev_err(cpu_dai->dev, "Invalid format!\n");
+ return -EINVAL;
+ }
+ regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
+ SUN50I_DMIC_RXFIFO_CTL_MODE,
+ SUN50I_DMIC_RXFIFO_CTL_MODE);
+
+ switch (rate) {
+ case 11025:
+ case 22050:
+ case 44100:
+ mclk = 22579200;
+ break;
+ case 8000:
+ case 12000:
+ case 16000:
+ case 24000:
+ case 32000:
+ case 48000:
+ mclk = 24576000;
+ break;
+ default:
+ dev_err(cpu_dai->dev, "Invalid rate!\n");
+ return -EINVAL;
+ }
+
+ if (clk_set_rate(host->dmic_clk, mclk)) {
+ dev_err(cpu_dai->dev, "mclk : %u not support\n", mclk);
+ 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_SAMPLE_RATE_MASK,
+ SUN50I_DMIC_SR_SAMPLE_RATE(dmic_rate_s[i].rate_bit));
+ break;
+ }
+ }
+
+ switch (params_physical_width(params)) {
+ case 16:
+ host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ break;
+ case 32:
+ host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ break;
+ default:
+ dev_err(cpu_dai->dev, "Unsupported physical sample width: %d\n",
+ params_physical_width(params));
+ 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:
+ /* 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);
+ break;
+
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ /* 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);
+ break;
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+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,
+};
+
+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)
+#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,
+ },
+ .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->bus_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->bus_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;
+
+ /* 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->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ if (IS_ERR(host->bus_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk),
+ "failed to get bus clock.\n");
+
+ host->dmic_clk = devm_clk_get(&pdev->dev, "mod");
+ 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;
+
+ platform_set_drvdata(pdev, host);
+
+ host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(host->rst))
+ return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
+ "Failed to get reset.\n");
+ 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.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
2021-07-11 12:20 [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver fengzheng923
@ 2021-07-15 7:47 ` Maxime Ripard
2021-08-01 9:47 ` 班涛
0 siblings, 1 reply; 4+ messages in thread
From: Maxime Ripard @ 2021-07-15 7:47 UTC (permalink / raw)
To: fengzheng923
Cc: lgirdwood, broonie, perex, tiwai, wens, jernej.skrabec, p.zabel,
samuel, krzk, linux-kernel, alsa-devel, linux-arm-kernel,
linux-sunxi
Hi
On Sun, Jul 11, 2021 at 08:20:55AM -0400, fengzheng923@gmail.com wrote:
> From: Ban Tao <fengzheng923@gmail.com>
>
> The Allwinner H6 and later SoCs have an DMIC block
> which is capable of capture.
>
> Signed-off-by: Ban Tao <fengzheng923@gmail.com>
>
> ---
> v1->v2:
> 1.Fix some compilation errors.
> 2.Modify some code styles.
> ---
> v2->v3:
> None.
> ---
> v3->v4:
> 1.add sig_bits.
> ---
> v4->v5:
> None.
> ---
> v5->v6:
> 1.Modify RXFIFO_CTL_MODE to mode 1.
> ---
> MAINTAINERS | 7 +
> sound/soc/sunxi/Kconfig | 8 +
> sound/soc/sunxi/Makefile | 1 +
> sound/soc/sunxi/sun50i-dmic.c | 403 ++++++++++++++++++++++++++++++++++
> 4 files changed, 419 insertions(+)
> create mode 100644 sound/soc/sunxi/sun50i-dmic.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e924f9e5df97..8d700baaa3ca 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..bbac836ba4de
> --- /dev/null
> +++ b/sound/soc/sunxi/sun50i-dmic.c
> @@ -0,0 +1,403 @@
> +// 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>
> +
> +#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_SAMPLE_RATE(v) ((v) << 0)
> + #define SUN50I_DMIC_SR_SAMPLE_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)
> +
> +
There's multiple blank lines here
> +struct sun50i_dmic_dev {
> + struct clk *dmic_clk;
> + struct clk *bus_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 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(asoc_rtd_to_cpu(rtd, 0));
> +
> + /* only support capture */
> + if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> + return -EINVAL;
> +
> + 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 mclk = 0;
> + unsigned int channels = params_channels(params);
> + struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
> + static struct dmic_rate dmic_rate_s[] = {
> + {44100, 0x0},
> + {48000, 0x0},
> + {22050, 0x2},
> + {24000, 0x2},
> + {11025, 0x4},
> + {12000, 0x4},
> + {32000, 0x1},
> + {16000, 0x3},
> + {8000, 0x5},
> + };
We should order these items. It looks like descending rate makes the
most sense?
Also, I'm not sure why we need to make that array local, can't this be a
global variable?
> + /* 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);
Do we need to store the channels bitmask in the main structure? It looks
fairly easy to generate, so I guess we could just use a macro
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S16_LE:
> + 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_RESOLUTION,
> + SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
> + break;
These two defines could be named a bit better, it's not really clear
what SUN50I_DMIC_RXFIFO_CTL_RESOLUTION means, exactly, as opposed to 0
(while it's actually the sample width).
What about something like SUN50I_DMIC_RXFIFO_CTL_SAMPLE_16 (for 0) and
_24 (for 1), while changing SUN50I_DMIC_RXFIFO_CTL_RESOLUTION for
SUN50I_DMIC_RXFIFO_CTL_SAMPLE_MASK ?
> + default:
> + dev_err(cpu_dai->dev, "Invalid format!\n");
> + return -EINVAL;
> + }
> + regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> + SUN50I_DMIC_RXFIFO_CTL_MODE,
> + SUN50I_DMIC_RXFIFO_CTL_MODE);
Same thing here, MODE doesn't really explain what this does, and why
we'd want to always set it.
I guess 0 is LSB_ZERO and 1 is MSB_SIGN?
> + switch (rate) {
> + case 11025:
> + case 22050:
> + case 44100:
> + mclk = 22579200;
> + break;
> + case 8000:
> + case 12000:
> + case 16000:
> + case 24000:
> + case 32000:
> + case 48000:
> + mclk = 24576000;
> + break;
> + default:
> + dev_err(cpu_dai->dev, "Invalid rate!\n");
> + return -EINVAL;
> + }
> +
> + if (clk_set_rate(host->dmic_clk, mclk)) {
> + dev_err(cpu_dai->dev, "mclk : %u not support\n", mclk);
> + 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_SAMPLE_RATE_MASK,
> + SUN50I_DMIC_SR_SAMPLE_RATE(dmic_rate_s[i].rate_bit));
> + break;
> + }
> + }
> +
> + switch (params_physical_width(params)) {
> + case 16:
> + host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> + break;
> + case 32:
> + host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> + break;
> + default:
> + dev_err(cpu_dai->dev, "Unsupported physical sample width: %d\n",
> + params_physical_width(params));
> + 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:
> + /* 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);
> + break;
Do we really need to clear the channel and global enable bits? and DRQ?
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + /* 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);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + return ret;
> +}
> +
> +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,
> +};
> +
> +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)
> +#define SUN50I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
SUN50I_DMIC_RATES has a tab between the define and its name, while
SUN50I_FORMATS has the tab after the name. We should be consistent.
Similarly, we should name both with the SUN50I_DMIC prefix.
> +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,
> + },
> + .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->bus_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;
A new line here would be great.
> + ret = clk_prepare_enable(host->bus_clk);
> + if (ret)
> + clk_disable_unprepare(host->dmic_clk);
> +
> + return ret;
In general we prefer to treat the error path and the success path
differently. In this case it would mean changing that part to
ret = clk_prepare_enable(host->bus_clk);
if (ret) {
clk_disable_unprepare(host->dmic_clk);
return ret;
}
return 0;
> +}
> +
> +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;
> +
> + /* 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);
Your second line should be aligned on the opening parenthesis
> + /* Clocks */
> + host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + if (IS_ERR(host->bus_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk),
> + "failed to get bus clock.\n");
> +
> + host->dmic_clk = devm_clk_get(&pdev->dev, "mod");
> + 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;
> +
> + platform_set_drvdata(pdev, host);
> +
> + host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(host->rst))
> + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
> + "Failed to get reset.\n");
Your binding states that the reset is mandatory so you don't need the
optional variant.
> + reset_control_deassert(host->rst);
Can't this be moved to the runtime_pm hooks?
> + ret = devm_snd_soc_register_component(&pdev->dev,
> + &sun50i_dmic_component, &sun50i_dmic_dai, 1);
Your second line should be aligned on the opening parenthesis
> + 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;
> + }
We have a depends on PM on some drivers already, so I guess it would
just make sense to add one more here instead of dealing with whether
runtime_pm is compiled in or not.
Also, the name of the label is misleading: it's called err_unregister
but you don't need to unregister anything and you actually disable
runtime_pm for that device.
err_disable_runtime_pm or something similar would be a better pick
Thanks!
Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
2021-07-15 7:47 ` Maxime Ripard
@ 2021-08-01 9:47 ` 班涛
2021-08-17 7:12 ` Maxime Ripard
0 siblings, 1 reply; 4+ messages in thread
From: 班涛 @ 2021-08-01 9:47 UTC (permalink / raw)
To: Maxime Ripard
Cc: lgirdwood, Mark Brown, perex, tiwai, wens, jernej.skrabec,
p.zabel, Samuel Holland, krzk, linux-kernel, alsa-devel,
linux-arm-kernel, linux-sunxi
Hi, Dear:
Maxime Ripard <maxime@cerno.tech> 于2021年7月15日周四 下午3:47写道：
>
> Hi
>
> On Sun, Jul 11, 2021 at 08:20:55AM -0400, fengzheng923@gmail.com wrote:
> > From: Ban Tao <fengzheng923@gmail.com>
> >
> > The Allwinner H6 and later SoCs have an DMIC block
> > which is capable of capture.
> >
> > Signed-off-by: Ban Tao <fengzheng923@gmail.com>
> >
> > ---
> > v1->v2:
> > 1.Fix some compilation errors.
> > 2.Modify some code styles.
> > ---
> > v2->v3:
> > None.
> > ---
> > v3->v4:
> > 1.add sig_bits.
> > ---
> > v4->v5:
> > None.
> > ---
> > v5->v6:
> > 1.Modify RXFIFO_CTL_MODE to mode 1.
> > ---
> > MAINTAINERS | 7 +
> > sound/soc/sunxi/Kconfig | 8 +
> > sound/soc/sunxi/Makefile | 1 +
> > sound/soc/sunxi/sun50i-dmic.c | 403 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 419 insertions(+)
> > create mode 100644 sound/soc/sunxi/sun50i-dmic.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e924f9e5df97..8d700baaa3ca 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..bbac836ba4de
> > --- /dev/null
> > +++ b/sound/soc/sunxi/sun50i-dmic.c
> > @@ -0,0 +1,403 @@
> > +// 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>
> > +
> > +#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_SAMPLE_RATE(v) ((v) << 0)
> > + #define SUN50I_DMIC_SR_SAMPLE_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)
> > +
> > +
>
> There's multiple blank lines here
>
> > +struct sun50i_dmic_dev {
> > + struct clk *dmic_clk;
> > + struct clk *bus_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 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(asoc_rtd_to_cpu(rtd, 0));
> > +
> > + /* only support capture */
> > + if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
> > + return -EINVAL;
> > +
> > + 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 mclk = 0;
> > + unsigned int channels = params_channels(params);
> > + struct sun50i_dmic_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
> > + static struct dmic_rate dmic_rate_s[] = {
> > + {44100, 0x0},
> > + {48000, 0x0},
> > + {22050, 0x2},
> > + {24000, 0x2},
> > + {11025, 0x4},
> > + {12000, 0x4},
> > + {32000, 0x1},
> > + {16000, 0x3},
> > + {8000, 0x5},
> > + };
>
> We should order these items. It looks like descending rate makes the
> most sense?
>
> Also, I'm not sure why we need to make that array local, can't this be a
> global variable?
>
> > + /* 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);
>
> Do we need to store the channels bitmask in the main structure? It looks
> fairly easy to generate, so I guess we could just use a macro
I need to store channels bitmask and use it in sun50i_dmic_trigger function.
>
> > + switch (params_format(params)) {
> > + case SNDRV_PCM_FORMAT_S16_LE:
> > + 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_RESOLUTION,
> > + SUN50I_DMIC_RXFIFO_CTL_RESOLUTION);
> > + break;
>
> These two defines could be named a bit better, it's not really clear
> what SUN50I_DMIC_RXFIFO_CTL_RESOLUTION means, exactly, as opposed to 0
> (while it's actually the sample width).
>
> What about something like SUN50I_DMIC_RXFIFO_CTL_SAMPLE_16 (for 0) and
> _24 (for 1), while changing SUN50I_DMIC_RXFIFO_CTL_RESOLUTION for
> SUN50I_DMIC_RXFIFO_CTL_SAMPLE_MASK ?
>
> > + default:
> > + dev_err(cpu_dai->dev, "Invalid format!\n");
> > + return -EINVAL;
> > + }
> > + regmap_update_bits(host->regmap, SUN50I_DMIC_RXFIFO_CTL,
> > + SUN50I_DMIC_RXFIFO_CTL_MODE,
> > + SUN50I_DMIC_RXFIFO_CTL_MODE);
>
> Same thing here, MODE doesn't really explain what this does, and why
> we'd want to always set it.
>
> I guess 0 is LSB_ZERO and 1 is MSB_SIGN?
Yes.
>
> > + switch (rate) {
> > + case 11025:
> > + case 22050:
> > + case 44100:
> > + mclk = 22579200;
> > + break;
> > + case 8000:
> > + case 12000:
> > + case 16000:
> > + case 24000:
> > + case 32000:
> > + case 48000:
> > + mclk = 24576000;
> > + break;
> > + default:
> > + dev_err(cpu_dai->dev, "Invalid rate!\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (clk_set_rate(host->dmic_clk, mclk)) {
> > + dev_err(cpu_dai->dev, "mclk : %u not support\n", mclk);
> > + 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_SAMPLE_RATE_MASK,
> > + SUN50I_DMIC_SR_SAMPLE_RATE(dmic_rate_s[i].rate_bit));
> > + break;
> > + }
> > + }
> > +
> > + switch (params_physical_width(params)) {
> > + case 16:
> > + host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> > + break;
> > + case 32:
> > + host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + break;
> > + default:
> > + dev_err(cpu_dai->dev, "Unsupported physical sample width: %d\n",
> > + params_physical_width(params));
> > + 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:
> > + /* 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);
> > + break;
>
> Do we really need to clear the channel and global enable bits? and DRQ?
Why not? I think we should clear them when not in use......
>
> > + case SNDRV_PCM_TRIGGER_STOP:
> > + case SNDRV_PCM_TRIGGER_SUSPEND:
> > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > + /* 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);
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + return ret;
> > +}
> > +
> > +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,
> > +};
> > +
> > +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)
> > +#define SUN50I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
>
> SUN50I_DMIC_RATES has a tab between the define and its name, while
> SUN50I_FORMATS has the tab after the name. We should be consistent.
> Similarly, we should name both with the SUN50I_DMIC prefix.
>
> > +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,
> > + },
> > + .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->bus_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;
>
> A new line here would be great.
>
> > + ret = clk_prepare_enable(host->bus_clk);
> > + if (ret)
> > + clk_disable_unprepare(host->dmic_clk);
> > +
> > + return ret;
>
> In general we prefer to treat the error path and the success path
> differently. In this case it would mean changing that part to
>
> ret = clk_prepare_enable(host->bus_clk);
> if (ret) {
> clk_disable_unprepare(host->dmic_clk);
> return ret;
> }
>
> return 0;
>
> > +}
> > +
> > +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;
> > +
> > + /* 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);
>
> Your second line should be aligned on the opening parenthesis
>
> > + /* Clocks */
> > + host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > + if (IS_ERR(host->bus_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk),
> > + "failed to get bus clock.\n");
> > +
> > + host->dmic_clk = devm_clk_get(&pdev->dev, "mod");
> > + 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;
> > +
> > + platform_set_drvdata(pdev, host);
> > +
> > + host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(host->rst))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
> > + "Failed to get reset.\n");
>
> Your binding states that the reset is mandatory so you don't need the
> optional variant.
>
> > + reset_control_deassert(host->rst);
>
> Can't this be moved to the runtime_pm hooks?
Is this necessary? I see that most of the driver files execute
reset_control_deassert in the probe function, and reset_control_assert
in the remove function.
>
> > + ret = devm_snd_soc_register_component(&pdev->dev,
> > + &sun50i_dmic_component, &sun50i_dmic_dai, 1);
>
> Your second line should be aligned on the opening parenthesis
>
> > + 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;
> > + }
>
> We have a depends on PM on some drivers already, so I guess it would
> just make sense to add one more here instead of dealing with whether
> runtime_pm is compiled in or not.
I don't understand. I am referring to the sun4i-spdif.c file. Which
driver files should I refer to?
>
> Also, the name of the label is misleading: it's called err_unregister
> but you don't need to unregister anything and you actually disable
> runtime_pm for that device.
>
> err_disable_runtime_pm or something similar would be a better pick
>
> Thanks!
> Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
2021-08-01 9:47 ` 班涛
@ 2021-08-17 7:12 ` Maxime Ripard
0 siblings, 0 replies; 4+ messages in thread
From: Maxime Ripard @ 2021-08-17 7:12 UTC (permalink / raw)
To: 班涛
Cc: lgirdwood, Mark Brown, perex, tiwai, wens, jernej.skrabec,
p.zabel, Samuel Holland, krzk, linux-kernel, alsa-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]
Hi,
On Sun, Aug 01, 2021 at 05:47:46PM +0800, 班涛 wrote:
> > > + /* 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);
> >
> > Do we need to store the channels bitmask in the main structure? It looks
> > fairly easy to generate, so I guess we could just use a macro
>
> I need to store channels bitmask and use it in sun50i_dmic_trigger function.
But you don't need it outside of hw_params. The channel setup is
typically done in hw_params, not in the trigger hook.
[...]
> > > + /* Clocks */
> > > + host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > > + if (IS_ERR(host->bus_clk))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk),
> > > + "failed to get bus clock.\n");
> > > +
> > > + host->dmic_clk = devm_clk_get(&pdev->dev, "mod");
> > > + 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;
> > > +
> > > + platform_set_drvdata(pdev, host);
> > > +
> > > + host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> > > + if (IS_ERR(host->rst))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
> > > + "Failed to get reset.\n");
> >
> > Your binding states that the reset is mandatory so you don't need the
> > optional variant.
> >
> > > + reset_control_deassert(host->rst);
> >
> > Can't this be moved to the runtime_pm hooks?
>
> Is this necessary? I see that most of the driver files execute
> reset_control_deassert in the probe function, and reset_control_assert
> in the remove function.
Your driver seems to not rely on the fact that the device remains
powered between each run anyway, so yeah, that way you will completely
power it down.
> >
> > > + ret = devm_snd_soc_register_component(&pdev->dev,
> > > + &sun50i_dmic_component, &sun50i_dmic_dai, 1);
> >
> > Your second line should be aligned on the opening parenthesis
> >
> > > + 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;
> > > + }
> >
> > We have a depends on PM on some drivers already, so I guess it would
> > just make sense to add one more here instead of dealing with whether
> > runtime_pm is compiled in or not.
>
> I don't understand. I am referring to the sun4i-spdif.c file. Which
> driver files should I refer to?
That whole logic is there to enable the device is CONFIG_PM is not
enabled (and thus the calls to pm_runtime_* won't do anything).
Just add a depends on PM to your Kconfig entry, and call
pm_runtime_enable there.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-17 7:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-11 12:20 [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver fengzheng923
2021-07-15 7:47 ` Maxime Ripard
2021-08-01 9:47 ` 班涛
2021-08-17 7:12 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).