Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
From: 班涛 <fengzheng923@gmail.com>
Date: 2021-08-01 09:48:00
Also in:
alsa-devel, linux-sunxi, lkml
Hi, Dear: Maxime Ripard [off-list ref] 于2021年7月15日周四 下午3:47写道：
Hi On Sun, Jul 11, 2021 at 08:20:55AM -0400, fengzheng923@gmail.com wrote:quoted
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.cdiff --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 REGMAPdiff --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.odiff --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 herequoted
+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?quoted
+ /* 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.
quoted
+ 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 ?quoted
+ 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.
quoted
+ 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......
quoted
+ 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.quoted
+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.quoted
+ 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;quoted
+} + +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 parenthesisquoted
+ /* 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.quoted
+ 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.
quoted
+ ret = devm_snd_soc_register_component(&pdev->dev, + &sun50i_dmic_component, &sun50i_dmic_dai, 1);Your second line should be aligned on the opening parenthesisquoted
+ 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
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel