[PATCH v1 0/2] Add PDM controller for StarFive JH8100 SoC

[PATCH v1 0/2] Add PDM controller for StarFive JH8100 SoC

[Xingyu Wu]

The Pulse Density Modulation (PDM) controller is a digital PDM
microphone interface controller and decoder that supports both
mono/stereo PDM format, and a Inter-IC Sound (I2S) transmitter
that outputs standard stereo audio data to another device. The
PDM controller includes two PDM modules, each PDM module can drive
one bitstream sampling clock and two bitstream coming data with
sampling clock rising and falling edge.
    
On the JH8100 SoC, PDM and I2S are fixedly connected as follow:
    PDM module 0 --> I2S channel 0
    PDM module 1 --> I2S channel 1

The first patch adds documentation to describe PDM controller
bindings. And the second patch adds PDM driver for the StarFive
JH8100 SoC. The DTS patch of PDM controller will be submitted
after the patchs of JH8100 DTS are accepted.

Xingyu Wu (2):
  dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC
  ASoC: starfive: Add PDM controller support

 .../bindings/sound/starfive,jh8100-pdm.yaml   |  84 ++++
 MAINTAINERS                                   |   7 +
 sound/soc/starfive/Kconfig                    |   7 +
 sound/soc/starfive/Makefile                   |   1 +
 sound/soc/starfive/jh8100_pdm.c               | 395 ++++++++++++++++++
 5 files changed, 494 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
 create mode 100644 sound/soc/starfive/jh8100_pdm.c

-- 
2.25.1

[PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Xingyu Wu]

Add bindings for the PDM controller for the StarFive JH8100 SoC.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../bindings/sound/starfive,jh8100-pdm.yaml   | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml

diff --git a/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
new file mode 100644
index 000000000000..a91b47d39ad3
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/starfive,jh8100-pdm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH8100 PDM controller
+
+description: |
+  The Pulse Density Modulation (PDM) controller is a digital PDM out
+  microphone interface controller and decoder that supports both
+  mono/stereo PDM format, and an Inter-IC Sound (I2S) transmitter that
+  outputs standard stereo audio data to another device. The I2S transmitter
+  can be configured to operate either a master or a slave (default mode).
+  The PDM controller includes two PDM modules, each PDM module can drive
+  one bitstream sampling clock and two bitstream coming data with sampling
+  clock rising and falling edge.
+
+maintainers:
+  - Xingyu Wu <xingyu.wu@starfivetech.com>
+  - Walker Chen <walker.chen@starfivetech.com>
+
+properties:
+  compatible:
+    const: starfive,jh8100-pdm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: DMIC output clock
+      - description: Main ICG clock
+
+  clock-names:
+    items:
+      - const: dmic
+      - const: icg
+
+  resets:
+    maxItems: 1
+
+  starfive,syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to System Register Controller sys_syscon_ne node.
+          - description: PDM source enabled control offset of SYS_SYSCON_NE register.
+          - description: PDM source enabled control mask
+    description:
+      The phandle to System Register Controller syscon node and the PDM source
+      from I2S enabled control offset and mask of SYS_SYSCON_NE register.
+
+  starfive,pdm-modulex:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    description:
+      The module x will be using in PDM controller. Default use module 0.
+
+  "#sound-dai-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - starfive,syscon
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pdm@12250000 {
+      compatible = "starfive,jh8100-pdm";
+      reg = <0x12250000 0x1000>;
+      clocks = <&syscrg_ne 142>,
+               <&syscrg_ne 171>;
+      clock-names = "dmic", "icg";
+      resets = <&syscrg_ne 44>;
+      starfive,syscon = <&sys_syscon_ne 0xC 0xFF>;
+      #sound-dai-cells = <0>;
+    };
-- 
2.25.1

[PATCH v1 2/2] ASoC: starfive: Add PDM controller support

[Xingyu Wu]

Add the driver of PDM controller for the StarFive JH8100 SoC.

The Pulse Density Modulation (PDM) controller is a digital PDM
microphone interface controller and decoder that supports both
mono/stereo PDM format, and a Inter-IC Sound (I2S) transmitter
that outputs standard stereo audio data to another device.

On the JH8100 SoC, PDM and I2S are fixedly connected as follow:
PDM module 0 --> I2S channel 0
PDM module 1 --> I2S channel 1

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 MAINTAINERS                     |   7 +
 sound/soc/starfive/Kconfig      |   7 +
 sound/soc/starfive/Makefile     |   1 +
 sound/soc/starfive/jh8100_pdm.c | 395 ++++++++++++++++++++++++++++++++
 4 files changed, 410 insertions(+)
 create mode 100644 sound/soc/starfive/jh8100_pdm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ecaaec6a6bf..7923f70d1192 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20941,6 +20941,13 @@ F:	Documentation/devicetree/bindings/power/starfive*
 F:	drivers/pmdomain/starfive/
 F:	include/dt-bindings/power/starfive,jh7110-pmu.h
 
+STARFIVE JH8100 PDM DRIVER
+M:	Xingyu Wu <xingyu.wu@starfivetech.com>
+M:	Walker Chen <walker.chen@starfivetech.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
+F:	sound/soc/starfive/jh8100_pdm.c
+
 STARFIVE SOC DRIVERS
 M:	Conor Dooley <conor@kernel.org>
 S:	Maintained
diff --git a/sound/soc/starfive/Kconfig b/sound/soc/starfive/Kconfig
index 279ac5c1d309..9beb734d2028 100644
--- a/sound/soc/starfive/Kconfig
+++ b/sound/soc/starfive/Kconfig
@@ -22,3 +22,10 @@ config SND_SOC_JH7110_TDM
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 	help
 	  Say Y or M if you want to add support for StarFive TDM driver.
+
+config SND_SOC_JH8100_PDM
+	tristate "JH8100 PDM controller device driver"
+	select SND_CADENCE_I2S_MC
+	select REGMAP_MMIO
+	help
+	  Say Y or M if you want to add support for StarFive PDM driver.
diff --git a/sound/soc/starfive/Makefile b/sound/soc/starfive/Makefile
index 9e958f70ef51..b672425830d9 100644
--- a/sound/soc/starfive/Makefile
+++ b/sound/soc/starfive/Makefile
@@ -1,3 +1,4 @@
 # StarFive Platform Support
 obj-$(CONFIG_SND_SOC_JH7110_PWMDAC) += jh7110_pwmdac.o
 obj-$(CONFIG_SND_SOC_JH7110_TDM) += jh7110_tdm.o
+obj-$(CONFIG_SND_SOC_JH8100_PDM) += jh8100_pdm.o
diff --git a/sound/soc/starfive/jh8100_pdm.c b/sound/soc/starfive/jh8100_pdm.c
new file mode 100644
index 000000000000..8a05a544e5ef
--- /dev/null
+++ b/sound/soc/starfive/jh8100_pdm.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PDM driver for the StarFive JH8100 SoC
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/tlv.h>
+
+/* PDM RES */
+/* MODULE 0 */
+#define JH8100_PDM_DMIC_CTRL0			0x00
+#define JH8100_PDM_DC_SCALE0			0x04
+/* MODULE 1 */
+#define JH8100_PDM_DMIC_CTRL1			0x10
+#define JH8100_PDM_DC_SCALE1			0x14
+#define JH8100_PDM_MODULEX_SHIFT		0x10
+
+/* PDM CTRL0/1 OFFSET */
+#define JH8100_PDM_DMIC_MSB_SHIFT		1
+#define JH8100_PDM_DMIC_MSB_MASK		GENMASK(3, 1)
+#define JH8100_PDM_DMIC_VOL_MASK		GENMASK(21, 16)
+#define JH8100_PDM_VOL_DB_MUTE			GENMASK(21, 16)
+#define JH8100_PDM_VOL_DB_MAX			0
+
+#define JH8100_PDM_DMIC_RVOL_MASK		BIT(22)
+#define JH8100_PDM_DMIC_LVOL_MASK		BIT(23)
+#define JH8100_PDM_DMIC_I2S_SLAVE		BIT(24)
+#define JH8100_PDM_DMIC_HPF_EN			BIT(28)
+#define JH8100_PDM_DMIC_FASTMODE_MASK		BIT(29)
+#define JH8100_PDM_DMIC_DC_BYPASS_MASK		BIT(30)
+#define JH8100_PDM_SW_RST_MASK			BIT(31)
+#define JH8100_PDM_SW_RST_RELEASE		BIT(31)
+
+/* PDM SCALE0/1 OFFSET */
+#define JH8100_DMIC_DCOFF3_MASK			GENMASK(27, 24)
+#define JH8100_DMIC_DCOFF3_DEF_VAL		GENMASK(27, 26)
+#define JH8100_DMIC_DCOFF1_MASK			GENMASK(15, 8)
+#define JH8100_DMIC_DCOFF1_SHIFT		8
+#define JH8100_DMIC_DCOFF1_DEF_VAL		FIELD_PREP(JH8100_DMIC_DCOFF1_MASK, 5)
+#define JH8100_DMIC_SCALE_MASK			GENMASK(5, 0)
+#define JH8100_DMIC_SCALE_DEF_VAL		0x8
+
+struct jh8100_pdm_priv {
+	struct regmap *regmap;
+	struct regmap *syscon_regmap;
+	struct device *dev;
+	struct clk *dmic_clk;
+	struct clk *icg_clk;
+	struct reset_control *rst;
+	unsigned int syscon_args[2]; /* [0]: offset, [1]: mask */
+};
+
+static const DECLARE_TLV_DB_SCALE(volume_tlv, -9450, 150, 0);
+
+static const struct snd_kcontrol_new jh8100_pdm_snd_controls[] = {
+	SOC_SINGLE("DC compensation Control", JH8100_PDM_DMIC_CTRL0, 30, 1, 0),
+	SOC_SINGLE("High Pass Filter Control", JH8100_PDM_DMIC_CTRL0, 28, 1, 0),
+	SOC_SINGLE("Left Channel Volume Control", JH8100_PDM_DMIC_CTRL0, 23, 1, 0),
+	SOC_SINGLE("Right Channel Volume Control", JH8100_PDM_DMIC_CTRL0, 22, 1, 0),
+	SOC_SINGLE_TLV("Volume", JH8100_PDM_DMIC_CTRL0, 16, 0x3F, 1, volume_tlv),
+	SOC_SINGLE("Data MSB Shift", JH8100_PDM_DMIC_CTRL0, 1, 7, 0),
+	SOC_SINGLE("SCALE", JH8100_PDM_DC_SCALE0, 0, 0x3F, 0),
+	SOC_SINGLE("DC offset", JH8100_PDM_DC_SCALE0, 8, 0xFFFFF, 0),
+};
+
+static void jh8100_pdm_enable(struct regmap *map)
+{
+	/* Left and Right Channel Volume Control Enable */
+	regmap_update_bits(map, JH8100_PDM_DMIC_CTRL0, JH8100_PDM_DMIC_RVOL_MASK, 0);
+	regmap_update_bits(map, JH8100_PDM_DMIC_CTRL0, JH8100_PDM_DMIC_LVOL_MASK, 0);
+}
+
+static void jh8100_pdm_disable(struct regmap *map)
+{
+	/* Left and Right Channel Volume Control Disable */
+	regmap_update_bits(map, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_RVOL_MASK, JH8100_PDM_DMIC_RVOL_MASK);
+	regmap_update_bits(map, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_LVOL_MASK, JH8100_PDM_DMIC_LVOL_MASK);
+}
+
+static int jh8100_pdm_dai_probe(struct snd_soc_dai *dai)
+{
+	struct jh8100_pdm_priv *priv = snd_soc_dai_get_drvdata(dai);
+
+	/* Change I2SDIN source to PDM */
+	regmap_update_bits(priv->syscon_regmap, priv->syscon_args[0],
+			   priv->syscon_args[1], priv->syscon_args[1]);
+
+	return 0;
+}
+
+static int jh8100_pdm_trigger(struct snd_pcm_substream *substream, int cmd,
+			      struct snd_soc_dai *dai)
+{
+	struct jh8100_pdm_priv *priv = snd_soc_dai_get_drvdata(dai);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		jh8100_pdm_enable(priv->regmap);
+		return 0;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		jh8100_pdm_disable(priv->regmap);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int jh8100_pdm_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct jh8100_pdm_priv *priv = snd_soc_dai_get_drvdata(dai);
+
+	/* set pdm_mclk,  PDM MCLK = 128 * LRCLK */
+	return clk_set_rate(priv->dmic_clk, 128 * params_rate(params));
+}
+
+static const struct snd_soc_dai_ops jh8100_pdm_dai_ops = {
+	.probe		= jh8100_pdm_dai_probe,
+	.trigger	= jh8100_pdm_trigger,
+	.hw_params	= jh8100_pdm_hw_params,
+};
+
+/* Use DMIC1 in PDM */
+static void jh8100_pdm_module_init(struct jh8100_pdm_priv *priv)
+{
+	/* Reset */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_SW_RST_MASK, 0x00);
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_SW_RST_MASK, JH8100_PDM_SW_RST_RELEASE);
+
+	/* Make sure the device is initially disabled */
+	jh8100_pdm_disable(priv->regmap);
+
+	/* MUTE */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_VOL_MASK, JH8100_PDM_VOL_DB_MUTE);
+
+	/* UNMUTE */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_VOL_MASK, JH8100_PDM_VOL_DB_MAX);
+
+	/* enable high pass filter */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_HPF_EN, JH8100_PDM_DMIC_HPF_EN);
+
+	/* PDM work as slave mode */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_I2S_SLAVE, JH8100_PDM_DMIC_I2S_SLAVE);
+
+	/* enable fast mode */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_FASTMODE_MASK, JH8100_PDM_DMIC_FASTMODE_MASK);
+
+	/* default dmic msb shift 0 */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DMIC_CTRL0,
+			   JH8100_PDM_DMIC_MSB_MASK, 0);
+
+	/* default scale: 0x8 */
+	regmap_update_bits(priv->regmap, JH8100_PDM_DC_SCALE0,
+			   JH8100_DMIC_SCALE_MASK, JH8100_DMIC_SCALE_DEF_VAL);
+
+	regmap_update_bits(priv->regmap, JH8100_PDM_DC_SCALE0,
+			   JH8100_DMIC_DCOFF1_MASK, JH8100_DMIC_DCOFF1_DEF_VAL);
+
+	regmap_update_bits(priv->regmap, JH8100_PDM_DC_SCALE0,
+			   JH8100_DMIC_DCOFF3_MASK, JH8100_DMIC_DCOFF3_DEF_VAL);
+}
+
+#define JH8100_PDM_RATES	(SNDRV_PCM_RATE_8000 | \
+				SNDRV_PCM_RATE_11025 | \
+				SNDRV_PCM_RATE_16000)
+
+#define JH8100_PDM_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+				SNDRV_PCM_FMTBIT_S32_LE)
+
+static struct snd_soc_dai_driver jh8100_pdm_dai_drv = {
+	.name = "PDM",
+	.id = 0,
+	.capture = {
+		.stream_name	= "Capture",
+		.channels_min	= 1,
+		.channels_max	= 2,
+		.rates		= JH8100_PDM_RATES,
+		.formats	= JH8100_PDM_FORMATS,
+	},
+	.ops = &jh8100_pdm_dai_ops,
+	.symmetric_rate = 1,
+};
+
+static int jh8100_pdm_component_probe(struct snd_soc_component *component)
+{
+	struct jh8100_pdm_priv *priv = snd_soc_component_get_drvdata(component);
+
+	snd_soc_component_init_regmap(component, priv->regmap);
+	snd_soc_add_component_controls(component, jh8100_pdm_snd_controls,
+				       ARRAY_SIZE(jh8100_pdm_snd_controls));
+
+	return 0;
+}
+
+static int jh8100_pdm_crg_enable(struct jh8100_pdm_priv *priv)
+{
+	int ret;
+
+	ret = clk_prepare_enable(priv->icg_clk);
+	if (ret)
+		return dev_err_probe(priv->dev, ret, "failed to enable icg clock\n");
+
+	ret = reset_control_deassert(priv->rst);
+	if (ret) {
+		dev_err(priv->dev, "failed to deassert pdm_apb\n");
+		goto disable_icg;
+	}
+
+	return 0;
+
+disable_icg:
+	clk_disable_unprepare(priv->icg_clk);
+	return ret;
+}
+
+#ifdef CONFIG_PM
+static int jh8100_pdm_runtime_suspend(struct device *dev)
+{
+	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->icg_clk);
+	return 0;
+}
+
+static int jh8100_pdm_runtime_resume(struct device *dev)
+{
+	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
+
+	return jh8100_pdm_crg_enable(priv);
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int jh8100_pdm_suspend(struct snd_soc_component *component)
+{
+	return pm_runtime_force_suspend(component->dev);
+}
+
+static int jh8100_pdm_resume(struct snd_soc_component *component)
+{
+	return pm_runtime_force_resume(component->dev);
+}
+
+#else
+#define jh8100_pdm_suspend	NULL
+#define jh8100_pdm_resume	NULL
+#endif
+
+static const struct snd_soc_component_driver jh8100_pdm_component_drv = {
+	.name = "jh8100-pdm",
+	.probe = jh8100_pdm_component_probe,
+	.suspend = jh8100_pdm_suspend,
+	.resume = jh8100_pdm_resume,
+};
+
+static const struct regmap_config jh8100_pdm_regmap_cfg = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= 0x20,
+};
+
+static int jh8100_pdm_crg_init(struct jh8100_pdm_priv *priv)
+{
+	priv->dmic_clk = devm_clk_get(priv->dev, "dmic");
+	if (IS_ERR(priv->dmic_clk))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->dmic_clk),
+				     "failed to get dmic clock.\n");
+
+	priv->icg_clk = devm_clk_get(priv->dev, "icg");
+	if (IS_ERR(priv->icg_clk))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->icg_clk),
+				     "failed to get icg clock.\n");
+
+	priv->rst = devm_reset_control_get_exclusive(priv->dev, NULL);
+	if (IS_ERR(priv->rst))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->rst), "failed to get pdm reset\n");
+
+	return jh8100_pdm_crg_enable(priv);
+}
+
+static int jh8100_pdm_probe(struct platform_device *pdev)
+{
+	struct jh8100_pdm_priv *priv;
+	void __iomem *base;
+	int ret;
+	u8 using_modulex;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	platform_set_drvdata(pdev, priv);
+
+	if (!device_property_read_u8(&pdev->dev, "starfive,pdm-modulex", &using_modulex))
+		if (using_modulex == 1)
+			base += JH8100_PDM_MODULEX_SHIFT; /* Use module 1 */
+
+	priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, &jh8100_pdm_regmap_cfg);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->regmap),
+				     "failed to init regmap\n");
+
+	priv->dev = &pdev->dev;
+	ret = jh8100_pdm_crg_init(priv);
+	if (ret)
+		return ret;
+
+	priv->syscon_regmap = syscon_regmap_lookup_by_phandle_args(pdev->dev.of_node,
+								   "starfive,syscon",
+								   2, priv->syscon_args);
+	if (IS_ERR(priv->syscon_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->syscon_regmap),
+				     "get the syscon regmap failed\n");
+
+	ret = devm_snd_soc_register_component(&pdev->dev, &jh8100_pdm_component_drv,
+					      &jh8100_pdm_dai_drv, 1);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to register pdm dai\n");
+
+	jh8100_pdm_module_init(priv);
+
+	pm_runtime_enable(&pdev->dev);
+	if (pm_runtime_enabled(&pdev->dev))
+		clk_disable_unprepare(priv->icg_clk);
+
+	return 0;
+}
+
+static int jh8100_pdm_remove(struct platform_device *pdev)
+{
+	struct jh8100_pdm_priv *priv = platform_get_drvdata(pdev);
+
+	/* Change I2SDIN source to default(PAD) */
+	regmap_update_bits(priv->syscon_regmap, priv->syscon_args[0],
+			   priv->syscon_args[1], 0);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id jh8100_pdm_of_match[] = {
+	{.compatible = "starfive,jh8100-pdm",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, jh8100_pdm_of_match);
+
+static const struct dev_pm_ops jh8100_pdm_pm_ops = {
+	SET_RUNTIME_PM_OPS(jh8100_pdm_runtime_suspend,
+			   jh8100_pdm_runtime_resume, NULL)
+};
+
+static struct platform_driver jh8100_pdm_driver = {
+	.driver = {
+		.name = "jh8100-pdm",
+		.of_match_table = jh8100_pdm_of_match,
+		.pm = &jh8100_pdm_pm_ops,
+	},
+	.probe = jh8100_pdm_probe,
+	.remove = jh8100_pdm_remove,
+};
+module_platform_driver(jh8100_pdm_driver);
+
+MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
+MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JH8100 PDM Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Krzysztof Kozlowski]

On 07/03/2024 04:37, Xingyu Wu wrote:
> Add bindings for the PDM controller for the StarFive JH8100 SoC.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../bindings/sound/starfive,jh8100-pdm.yaml   | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> new file mode 100644
> index 000000000000..a91b47d39ad3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/starfive,jh8100-pdm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH8100 PDM controller
> +
> +description: |
> +  The Pulse Density Modulation (PDM) controller is a digital PDM out
> +  microphone interface controller and decoder that supports both
> +  mono/stereo PDM format, and an Inter-IC Sound (I2S) transmitter that
> +  outputs standard stereo audio data to another device. The I2S transmitter
> +  can be configured to operate either a master or a slave (default mode).
> +  The PDM controller includes two PDM modules, each PDM module can drive
> +  one bitstream sampling clock and two bitstream coming data with sampling
> +  clock rising and falling edge.
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +  - Walker Chen <walker.chen@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh8100-pdm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: DMIC output clock
> +      - description: Main ICG clock
> +
> +  clock-names:
> +    items:
> +      - const: dmic
> +      - const: icg
> +
> +  resets:
> +    maxItems: 1
> +
> +  starfive,syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller sys_syscon_ne node.
> +          - description: PDM source enabled control offset of SYS_SYSCON_NE register.
> +          - description: PDM source enabled control mask
> +    description:
> +      The phandle to System Register Controller syscon node and the PDM source
> +      from I2S enabled control offset and mask of SYS_SYSCON_NE register.
> +
> +  starfive,pdm-modulex:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description:
> +      The module x will be using in PDM controller. Default use module 0.

This is an index of the block instance? If so, then it's not allowed.
Otherwise I don't understand the description.

Anyway, don't repeat constraints in free form text. default: 0, if this
is going to stay.

> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +  - starfive,syscon
> +
> +unevaluatedProperties: false

This is wrong without $ref, which points you to missing $ref to dai-common.

> +
> +examples:
> +  - |
> +    pdm@12250000 {
> +      compatible = "starfive,jh8100-pdm";
> +      reg = <0x12250000 0x1000>;
> +      clocks = <&syscrg_ne 142>,
> +               <&syscrg_ne 171>;
> +      clock-names = "dmic", "icg";
> +      resets = <&syscrg_ne 44>;
> +      starfive,syscon = <&sys_syscon_ne 0xC 0xFF>;

Lowercase hex only.

> +      #sound-dai-cells = <0>;
> +    };

Best regards,
Krzysztof

Re: [PATCH v1 2/2] ASoC: starfive: Add PDM controller support

[Mark Brown]

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

On Thu, Mar 07, 2024 at 11:37:08AM +0800, Xingyu Wu wrote:

> +static const struct snd_kcontrol_new jh8100_pdm_snd_controls[] = {
> +	SOC_SINGLE("DC compensation Control", JH8100_PDM_DMIC_CTRL0, 30, 1, 0),
> +	SOC_SINGLE("High Pass Filter Control", JH8100_PDM_DMIC_CTRL0, 28, 1, 0),
> +	SOC_SINGLE("Left Channel Volume Control", JH8100_PDM_DMIC_CTRL0, 23, 1, 0),
> +	SOC_SINGLE("Right Channel Volume Control", JH8100_PDM_DMIC_CTRL0, 22, 1, 0),
> +	SOC_SINGLE_TLV("Volume", JH8100_PDM_DMIC_CTRL0, 16, 0x3F, 1, volume_tlv),
> +	SOC_SINGLE("Data MSB Shift", JH8100_PDM_DMIC_CTRL0, 1, 7, 0),
> +	SOC_SINGLE("SCALE", JH8100_PDM_DC_SCALE0, 0, 0x3F, 0),
> +	SOC_SINGLE("DC offset", JH8100_PDM_DC_SCALE0, 8, 0xFFFFF, 0),
> +};

Simple on/off switches should have names ending in Switch, volumes
should end in Volume as per control-names.rst.  Please for the next
version you post show the output of running mixer-test on a system with
this device, it will identify these and other issues.

> +static int jh8100_pdm_component_probe(struct snd_soc_component *component)
> +{
> +	struct jh8100_pdm_priv *priv = snd_soc_component_get_drvdata(component);
> +
> +	snd_soc_component_init_regmap(component, priv->regmap);
> +	snd_soc_add_component_controls(component, jh8100_pdm_snd_controls,
> +				       ARRAY_SIZE(jh8100_pdm_snd_controls));

You can just specify the controls in the snd_soc_compoenent_driver.

> +#ifdef CONFIG_PM
> +static int jh8100_pdm_runtime_suspend(struct device *dev)
> +{
> +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(priv->icg_clk);
> +	return 0;
> +}
> +
> +static int jh8100_pdm_runtime_resume(struct device *dev)
> +{
> +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> +
> +	return jh8100_pdm_crg_enable(priv);
> +}
> +#endif

It's weird that the runtime suspend and resume are not symmetric - why
do we need to bring the device out of reset but not put it into reset?

> +	if (!device_property_read_u8(&pdev->dev, "starfive,pdm-modulex", &using_modulex))
> +		if (using_modulex == 1)
> +			base += JH8100_PDM_MODULEX_SHIFT; /* Use module 1 */

This really looks like you've got one hardware block with two devices in
it, either the address ranges registered for the devices in DT should be
separate and you shouldn't need this property or you should have one
component registering both PDM interfaces.

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

回复: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Xingyu Wu]

> On 07/03/2024 04:37, Xingyu Wu wrote:
> > Add bindings for the PDM controller for the StarFive JH8100 SoC.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your
> patch is touching.
> 

Okay, will fix.

> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > ---
> >  .../bindings/sound/starfive,jh8100-pdm.yaml   | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> > b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> > new file mode 100644
> > index 000000000000..a91b47d39ad3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/starfive,jh8100-pdm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive JH8100 PDM controller
> > +
> > +description: |
> > +  The Pulse Density Modulation (PDM) controller is a digital PDM out
> > +  microphone interface controller and decoder that supports both
> > +  mono/stereo PDM format, and an Inter-IC Sound (I2S) transmitter
> > +that
> > +  outputs standard stereo audio data to another device. The I2S
> > +transmitter
> > +  can be configured to operate either a master or a slave (default mode).
> > +  The PDM controller includes two PDM modules, each PDM module can
> > +drive
> > +  one bitstream sampling clock and two bitstream coming data with
> > +sampling
> > +  clock rising and falling edge.
> > +
> > +maintainers:
> > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > +  - Walker Chen <walker.chen@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: starfive,jh8100-pdm
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: DMIC output clock
> > +      - description: Main ICG clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: dmic
> > +      - const: icg
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  starfive,syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to System Register Controller
> sys_syscon_ne node.
> > +          - description: PDM source enabled control offset of
> SYS_SYSCON_NE register.
> > +          - description: PDM source enabled control mask
> > +    description:
> > +      The phandle to System Register Controller syscon node and the PDM
> source
> > +      from I2S enabled control offset and mask of SYS_SYSCON_NE register.
> > +
> > +  starfive,pdm-modulex:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +    description:
> > +      The module x will be using in PDM controller. Default use module 0.
> 
> This is an index of the block instance? If so, then it's not allowed.
> Otherwise I don't understand the description.
> 

No, this is just one instance. The PDM have two internal and independent modules or called channels.
They can be configured and used separately, and the user can choose which channel to use.

> Anyway, don't repeat constraints in free form text. default: 0, if this is going to
> stay.
> 

Oh, I just want to describe that if no this property, the driver default use module 0.
I will make this clear in next version.

> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - starfive,syscon
> > +
> > +unevaluatedProperties: false
> 
> This is wrong without $ref, which points you to missing $ref to dai-common.

Will add it. Thanks.

> 
> > +
> > +examples:
> > +  - |
> > +    pdm@12250000 {
> > +      compatible = "starfive,jh8100-pdm";
> > +      reg = <0x12250000 0x1000>;
> > +      clocks = <&syscrg_ne 142>,
> > +               <&syscrg_ne 171>;
> > +      clock-names = "dmic", "icg";
> > +      resets = <&syscrg_ne 44>;
> > +      starfive,syscon = <&sys_syscon_ne 0xC 0xFF>;
> 
> Lowercase hex only.

Will fix.

> 
> > +      #sound-dai-cells = <0>;
> > +    };
> 
> Best regards,
> Krzysztof

Thanks,
Xingyu Wu

Re: 回复: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Krzysztof Kozlowski]

On 08/03/2024 08:49, Xingyu Wu wrote:
>>> +
>>> +  starfive,pdm-modulex:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1]
>>> +    description:
>>> +      The module x will be using in PDM controller. Default use module 0.
>>
>> This is an index of the block instance? If so, then it's not allowed.
>> Otherwise I don't understand the description.
>>
> 
> No, this is just one instance. The PDM have two internal and independent modules or called channels.
> They can be configured and used separately, and the user can choose which channel to use.
> 

Do the modulex differ? Why different boards would choose one over another?


Best regards,
Krzysztof

回复: [PATCH v1 2/2] ASoC: starfive: Add PDM controller support

[Xingyu Wu]

> 
> On Thu, Mar 07, 2024 at 11:37:08AM +0800, Xingyu Wu wrote:
> 
> > +static const struct snd_kcontrol_new jh8100_pdm_snd_controls[] = {
> > +	SOC_SINGLE("DC compensation Control", JH8100_PDM_DMIC_CTRL0, 30,
> 1, 0),
> > +	SOC_SINGLE("High Pass Filter Control", JH8100_PDM_DMIC_CTRL0, 28, 1,
> 0),
> > +	SOC_SINGLE("Left Channel Volume Control", JH8100_PDM_DMIC_CTRL0,
> 23, 1, 0),
> > +	SOC_SINGLE("Right Channel Volume Control", JH8100_PDM_DMIC_CTRL0,
> 22, 1, 0),
> > +	SOC_SINGLE_TLV("Volume", JH8100_PDM_DMIC_CTRL0, 16, 0x3F, 1,
> volume_tlv),
> > +	SOC_SINGLE("Data MSB Shift", JH8100_PDM_DMIC_CTRL0, 1, 7, 0),
> > +	SOC_SINGLE("SCALE", JH8100_PDM_DC_SCALE0, 0, 0x3F, 0),
> > +	SOC_SINGLE("DC offset", JH8100_PDM_DC_SCALE0, 8, 0xFFFFF, 0), };
> 
> Simple on/off switches should have names ending in Switch, volumes should end
> in Volume as per control-names.rst.  Please for the next version you post show
> the output of running mixer-test on a system with this device, it will identify
> these and other issues.

Will fix. Thanks.

> 
> > +static int jh8100_pdm_component_probe(struct snd_soc_component
> > +*component) {
> > +	struct jh8100_pdm_priv *priv =
> > +snd_soc_component_get_drvdata(component);
> > +
> > +	snd_soc_component_init_regmap(component, priv->regmap);
> > +	snd_soc_add_component_controls(component, jh8100_pdm_snd_controls,
> > +				       ARRAY_SIZE(jh8100_pdm_snd_controls));
> 
> You can just specify the controls in the snd_soc_compoenent_driver.

It seem to be more convenient. Noted.

> 
> > +#ifdef CONFIG_PM
> > +static int jh8100_pdm_runtime_suspend(struct device *dev) {
> > +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(priv->icg_clk);
> > +	return 0;
> > +}
> > +
> > +static int jh8100_pdm_runtime_resume(struct device *dev) {
> > +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> > +
> > +	return jh8100_pdm_crg_enable(priv);
> > +}
> > +#endif
> 
> It's weird that the runtime suspend and resume are not symmetric - why do we
> need to bring the device out of reset but not put it into reset?

I will add the reset when suspend.

> 
> > +	if (!device_property_read_u8(&pdev->dev, "starfive,pdm-modulex",
> &using_modulex))
> > +		if (using_modulex == 1)
> > +			base += JH8100_PDM_MODULEX_SHIFT; /* Use module 1 */
> 
> This really looks like you've got one hardware block with two devices in it, either
> the address ranges registered for the devices in DT should be separate and you
> shouldn't need this property or you should have one component registering both
> PDM interfaces.

Yeah, They like two independent device and have different register to configure, but just use the same clocks and resets.
Due to the sample rate depend on the share clocks, they should be registered together as a 4-channel capture device (rarely used), or just one of them can be registered separately as a 2-channel device.
BTW, can I use the 0x12250000 about the property of reg for device 0 or 0x12250010 for device 1 to choose which device to be used in DT?

Best regards,
Xingyu Wu

回复: 回复: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Xingyu Wu]

> On 08/03/2024 08:49, Xingyu Wu wrote:
> >>> +
> >>> +  starfive,pdm-modulex:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [0, 1]
> >>> +    description:
> >>> +      The module x will be using in PDM controller. Default use module 0.
> >>
> >> This is an index of the block instance? If so, then it's not allowed.
> >> Otherwise I don't understand the description.
> >>
> >
> > No, this is just one instance. The PDM have two internal and independent
> modules or called channels.
> > They can be configured and used separately, and the user can choose which
> channel to use.
> >
> 
> Do the modulex differ? Why different boards would choose one over another?
> 

They are same. The choice between them is base on the match with I2S.
The DMA data channel of hardware between them is fixed linked:
PDM module 0 --> I2S channel 0,
PDM module 1 --> I2S channel 1
I2S uses higher-number channels first for capture (like channel 1), so PDM should skips module 0 and uses module 1.
Oh, I just thought of a way to fix them that change the priority of I2S channel to use lower-number channels first and PDM need not skip module0.

Best regards,
Xingyu Wu

Re: 回复: 回复: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Krzysztof Kozlowski]

On 08/03/2024 10:19, Xingyu Wu wrote:
>> On 08/03/2024 08:49, Xingyu Wu wrote:
>>>>> +
>>>>> +  starfive,pdm-modulex:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum: [0, 1]
>>>>> +    description:
>>>>> +      The module x will be using in PDM controller. Default use module 0.
>>>>
>>>> This is an index of the block instance? If so, then it's not allowed.
>>>> Otherwise I don't understand the description.
>>>>
>>>
>>> No, this is just one instance. The PDM have two internal and independent
>> modules or called channels.
>>> They can be configured and used separately, and the user can choose which
>> channel to use.
>>>
>>
>> Do the modulex differ? Why different boards would choose one over another?
>>
> 
> They are same. The choice between them is base on the match with I2S.
> The DMA data channel of hardware between them is fixed linked:
> PDM module 0 --> I2S channel 0,
> PDM module 1 --> I2S channel 1
> I2S uses higher-number channels first for capture (like channel 1), so PDM should skips module 0 and uses module 1.
> Oh, I just thought of a way to fix them that change the priority of I2S channel to use lower-number channels first and PDM need not skip module0.
> 

Hm, then maybe this should be somehow linked with choice of I2C channel?
Do you have anywhere a link to complete DTS with sound card?

Best regards,
Krzysztof

Re: 回复: [PATCH v1 2/2] ASoC: starfive: Add PDM controller support

[Mark Brown]

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

On Fri, Mar 08, 2024 at 08:45:45AM +0000, Xingyu Wu wrote:
> > On Thu, Mar 07, 2024 at 11:37:08AM +0800, Xingyu Wu wrote:

> > > +	if (!device_property_read_u8(&pdev->dev, "starfive,pdm-modulex",
> > &using_modulex))
> > > +		if (using_modulex == 1)
> > > +			base += JH8100_PDM_MODULEX_SHIFT; /* Use module 1 */

> > This really looks like you've got one hardware block with two devices in it, either
> > the address ranges registered for the devices in DT should be separate and you
> > shouldn't need this property or you should have one component registering both
> > PDM interfaces.

> Yeah, They like two independent device and have different register to
> configure, but just use the same clocks and resets.  Due to the sample
> rate depend on the share clocks, they should be registered together as
> a 4-channel capture device (rarely used), or just one of them can be
> registered separately as a 2-channel device.  BTW, can I use the
> 0x12250000 about the property of reg for device 0 or 0x12250010 for
> device 1 to choose which device to be used in DT?

Ah, so it's actually a small MFD but given that it's two audio blocks
possibly not worth registering as such.  I'd register two stereo DAIs to
one component and then use the DAI ID to figure out which registers to
write to.  Four channel mode might need a property to put everything as
one DAI and not register the second one, or it might just be OK to let
the first DAI be 4 channel with runtime error checking.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

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