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

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