Message-ID: <20210715074750.ewbggulc5kast6ez@gilmour>
Date:   Thu, 15 Jul 2021 09:47:50 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     fengzheng923@...il.com
Cc:     lgirdwood@...il.com, broonie@...nel.org, perex@...ex.cz,
        tiwai@...e.com, wens@...e.org, jernej.skrabec@...il.com,
        p.zabel@...gutronix.de, samuel@...lland.org, krzk@...nel.org,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver

Hi

On Sun, Jul 11, 2021 at 08:20:55AM -0400, fengzheng923@...il.com wrote:
> From: Ban Tao <fengzheng923@...il.com>
> 
> The Allwinner H6 and later SoCs have an DMIC block
> which is capable of capture.
> 
> Signed-off-by: Ban Tao <fengzheng923@...il.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@...r.kernel.org
>  S:	Maintained
>  F:	drivers/staging/media/sunxi/cedrus/
>  
> +ALLWINNER DMIC DRIVERS
> +M:	Ban Tao <fengzheng923@...il.com>
> +L:	alsa-devel@...a-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@...ddle.net>
>  M:	Ivan Kokshaysky <ink@...assic.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@...il.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

Powered by blists - more mailing lists