[PATCH 0/9] Add Chameleon v3 ASoC audio

[PATCH 0/9] Add Chameleon v3 ASoC audio

[Paweł Anikiel]

The Google Chameleon v3 is a device made for testing audio and video
paths of other devices. This patchset adds support for ASoC audio on
this device. It has two audio sources: HDMI audio from the it68051 chip
(RX only), and analog audio from the ssm2603 chip (RX and TX).

The patchset adds the ASoC platform and machine drivers, as well as some
changes to the existing ssm2602 codec driver.

Paweł Anikiel (9):
  ASoC: Add Chameleon v3 audio
  dt-bindings: ASoC: Add chv3-i2s
  dt-bindings: ASoC: Add chv3-audio
  dt-bindings: ASoC: Add it68051
  ASoC: ssm2602: Add workaround for playback with external MCLK
  ASoC: ssm2602: Add support for CLKDIV2
  ASoC: ssm2602: Add mute gpio
  dt-bindings: ASoC: Add mute-gpio to ssm2602
  ARM: dts: chameleonv3: Add ssm2603 mute gpio

 .../devicetree/bindings/sound/adi,ssm2602.txt |   4 +
 .../bindings/sound/google,chv3-audio.yaml     |  49 +++
 .../bindings/sound/google,chv3-i2s.yaml       |  42 +++
 .../bindings/sound/ite,it68051.yaml           |  23 ++
 .../boot/dts/socfpga_arria10_chameleonv3.dts  |  30 ++
 sound/soc/Kconfig                             |   1 +
 sound/soc/Makefile                            |   1 +
 sound/soc/chameleonv3/Kconfig                 |   7 +
 sound/soc/chameleonv3/Makefile                |   2 +
 sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
 sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
 sound/soc/chameleonv3/chv3-it68051.c          |  41 +++
 sound/soc/codecs/ssm2602.c                    |  37 +-
 13 files changed, 692 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/ite,it68051.yaml
 create mode 100644 sound/soc/chameleonv3/Kconfig
 create mode 100644 sound/soc/chameleonv3/Makefile
 create mode 100644 sound/soc/chameleonv3/chv3-audio.c
 create mode 100644 sound/soc/chameleonv3/chv3-i2s.c
 create mode 100644 sound/soc/chameleonv3/chv3-it68051.c

-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH 1/9] ASoC: Add Chameleon v3 audio

[Paweł Anikiel]

Add machine and platform drivers for ASoC audio on Chameleon v3.

The board has two audio sources: HDMI audio from the it68051 chip
(RX only), and analog audio from the ssm2603 chip (RX and TX).

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 .../boot/dts/socfpga_arria10_chameleonv3.dts  |  28 ++
 sound/soc/Kconfig                             |   1 +
 sound/soc/Makefile                            |   1 +
 sound/soc/chameleonv3/Kconfig                 |   7 +
 sound/soc/chameleonv3/Makefile                |   2 +
 sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
 sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
 sound/soc/chameleonv3/chv3-it68051.c          |  41 +++
 8 files changed, 538 insertions(+)
 create mode 100644 sound/soc/chameleonv3/Kconfig
 create mode 100644 sound/soc/chameleonv3/Makefile
 create mode 100644 sound/soc/chameleonv3/chv3-audio.c
 create mode 100644 sound/soc/chameleonv3/chv3-i2s.c
 create mode 100644 sound/soc/chameleonv3/chv3-it68051.c

diff --git a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
index 422d00cd4c74..c75f41058196 100644
--- a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
+++ b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
@@ -15,6 +15,34 @@ aliases {
 		i2c0 = &i2c0;
 		i2c1 = &i2c1;
 	};
+
+	soc {
+		i2s0: i2s@c0060300 {
+			compatible = "google,chv3-i2s";
+			reg = <0xc0060300 0x100>,
+			      <0xc0060f00 0x10>;
+			interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		i2s1: i2s@c0060400 {
+			compatible = "google,chv3-i2s";
+			reg = <0xc0060400 0x100>,
+			      <0xc0060f10 0x10>;
+			interrupts = <0 21 IRQ_TYPE_LEVEL_HIGH>;
+		};
+	};
+
+	sound {
+		compatible = "google,chv3-audio";
+		google,audio-cpu0 = <&i2s1>;
+		google,audio-codec0 = <&ssm2603>;
+		google,audio-cpu1 = <&i2s0>;
+		google,audio-codec1 = <&it68051>;
+	};
+
+	it68051: audio-codec {
+		compatible = "ite,it68051";
+	};
 };
 
 &gmac0 {
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 848fbae26c3b..d5a24819a0eb 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -72,6 +72,7 @@ source "sound/soc/apple/Kconfig"
 source "sound/soc/atmel/Kconfig"
 source "sound/soc/au1x/Kconfig"
 source "sound/soc/bcm/Kconfig"
+source "sound/soc/chameleonv3/Kconfig"
 source "sound/soc/cirrus/Kconfig"
 source "sound/soc/dwc/Kconfig"
 source "sound/soc/fsl/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 507eaed1d6a1..5572f4ebe59e 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_SND_SOC)	+= amd/
 obj-$(CONFIG_SND_SOC)	+= atmel/
 obj-$(CONFIG_SND_SOC)	+= au1x/
 obj-$(CONFIG_SND_SOC)	+= bcm/
+obj-$(CONFIG_SND_SOC)	+= chameleonv3/
 obj-$(CONFIG_SND_SOC)	+= cirrus/
 obj-$(CONFIG_SND_SOC)	+= dwc/
 obj-$(CONFIG_SND_SOC)	+= fsl/
diff --git a/sound/soc/chameleonv3/Kconfig b/sound/soc/chameleonv3/Kconfig
new file mode 100644
index 000000000000..8386c44d6e92
--- /dev/null
+++ b/sound/soc/chameleonv3/Kconfig
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config SND_SOC_CHV3
+	tristate "SoC Audio support for Chameleon v3"
+	select SND_SOC_SSM2602
+	select SND_SOC_SSM2602_I2C
+	help
+	  Say Y if you want to add audio support for the Chameleon v3.
diff --git a/sound/soc/chameleonv3/Makefile b/sound/soc/chameleonv3/Makefile
new file mode 100644
index 000000000000..a542ad412b93
--- /dev/null
+++ b/sound/soc/chameleonv3/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_SND_SOC_CHV3) += chv3-i2s.o chv3-audio.o chv3-it68051.o
diff --git a/sound/soc/chameleonv3/chv3-audio.c b/sound/soc/chameleonv3/chv3-audio.c
new file mode 100644
index 000000000000..e860447c2cef
--- /dev/null
+++ b/sound/soc/chameleonv3/chv3-audio.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <sound/soc.h>
+
+static int chv3_ssm2603_hw_params(struct snd_pcm_substream *substream,
+			  struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
+
+	return snd_soc_dai_set_sysclk(dai, 0, 22579200, SND_SOC_CLOCK_IN);
+}
+
+static const struct snd_soc_ops chv3_ssm2603_ops = {
+	.hw_params = chv3_ssm2603_hw_params,
+};
+
+SND_SOC_DAILINK_DEFS(ssm2603,
+	DAILINK_COMP_ARRAY(COMP_CPU("chv3-i2s")),
+	DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "ssm2602-hifi")), /* 2602 - not a typo */
+	DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+SND_SOC_DAILINK_DEFS(it68051,
+	DAILINK_COMP_ARRAY(COMP_CPU("chv3-i2s")),
+	DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "it68051-hifi")),
+	DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+static struct snd_soc_dai_link chv3_audio_dais[] = {
+{
+	.name = "ssm2603",
+	.stream_name = "ssm2603 PCM",
+	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
+	.ops = &chv3_ssm2603_ops,
+	SND_SOC_DAILINK_REG(ssm2603),
+},
+{
+	.name = "it68051",
+	.stream_name = "it68051 PCM",
+	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
+	SND_SOC_DAILINK_REG(it68051),
+},
+};
+
+static struct snd_soc_card chv3_audio_card = {
+	.name = "chv3-audio",
+	.owner = THIS_MODULE,
+	.dai_link = chv3_audio_dais,
+	.num_links = 2,
+};
+
+static int chv3_audio_probe(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = &chv3_audio_card;
+	struct device_node *np;
+
+	card->dev = &pdev->dev;
+
+	np = of_parse_phandle(pdev->dev.of_node, "google,audio-cpu0", 0);
+	if (!np) {
+		dev_err(&pdev->dev, "no google,audio-cpu0 property\n");
+		return -EINVAL;
+	}
+	chv3_audio_dais[0].cpus->of_node = np;
+	chv3_audio_dais[0].platforms->of_node = np;
+
+	np = of_parse_phandle(pdev->dev.of_node, "google,audio-codec0", 0);
+	if (!np) {
+		dev_err(&pdev->dev, "no google,audio-codec0 property\n");
+		return -EINVAL;
+	}
+	chv3_audio_dais[0].codecs->of_node = np;
+
+	np = of_parse_phandle(pdev->dev.of_node, "google,audio-cpu1", 0);
+	if (!np) {
+		dev_err(&pdev->dev, "no google,audio-cpu1 property\n");
+		return -EINVAL;
+	}
+	chv3_audio_dais[1].cpus->of_node = np;
+	chv3_audio_dais[1].platforms->of_node = np;
+
+	np = of_parse_phandle(pdev->dev.of_node, "google,audio-codec1", 0);
+	if (!np) {
+		dev_err(&pdev->dev, "no google,audio-codec1 property\n");
+		return -EINVAL;
+	}
+	chv3_audio_dais[1].codecs->of_node = np;
+
+	return devm_snd_soc_register_card(card->dev, card);
+}
+
+static const struct of_device_id chv3_audio_dt_ids[] = {
+	{ .compatible = "google,chv3-audio" },
+	{ },
+};
+
+static struct platform_driver chv3_audio_driver = {
+	.driver = {
+		.name = "chv3-audio",
+		.of_match_table = of_match_ptr(chv3_audio_dt_ids),
+	},
+	.probe = chv3_audio_probe,
+};
+
+module_platform_driver(chv3_audio_driver);
+
+MODULE_AUTHOR("Pawel Anikiel <pan@semihalf.com>");
+MODULE_DESCRIPTION("ALSA SoC Chameleon v3 audio driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/chameleonv3/chv3-i2s.c b/sound/soc/chameleonv3/chv3-i2s.c
new file mode 100644
index 000000000000..e946bff2e6fc
--- /dev/null
+++ b/sound/soc/chameleonv3/chv3-i2s.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <sound/soc.h>
+
+/*
+ * The I2S interface consists of two ring buffers - one for RX and one for
+ * TX.  A ring buffer has a producer index and a consumer index. Depending
+ * on which way the data is flowing, either the software or the hardware
+ * writes data and updates the producer index, and the other end reads data
+ * and updates the consumer index.
+ *
+ * The pointer managed by software is updated using the .ack callback
+ * (see chv3_dma_ack). This seems to be the only way to reliably obtain
+ * the appl_ptr from within the driver and pass it to hardware.
+ *
+ * Because of the two pointer design, the ring buffer can never be full. With
+ * capture this isn't a problem, because the hardware being the producer
+ * will wait for the consumer index to move out of the way.  With playback,
+ * however, this is problematic, because ALSA wants to fill up the buffer
+ * completely when waiting for hardware. In the .ack callback, the driver
+ * would have to wait for the consumer index to move out of the way by
+ * busy-waiting, which would keep stalling the kernel for quite a long time.
+ *
+ * The workaround to this problem is to "lie" to ALSA that the hw_pointer
+ * is one period behind what it actually is (see chv3_dma_pointer). This
+ * way, ALSA will not try to fill up the entire buffer, and all callbacks
+ * are wait-free.
+ */
+
+#define I2S_TX_ENABLE		0x00
+#define I2S_TX_BASE_ADDR	0x04
+#define I2S_TX_BUFFER_SIZE	0x08
+#define I2S_TX_PRODUCER_IDX	0x0c
+#define I2S_TX_CONSUMER_IDX	0x10
+#define I2S_RX_ENABLE		0x14
+#define I2S_RX_BASE_ADDR	0x18
+#define I2S_RX_BUFFER_SIZE	0x1c
+#define I2S_RX_PRODUCER_IDX	0x20
+#define I2S_RX_CONSUMER_IDX	0x24
+
+#define I2S_SOFT_RESET		0x2c
+#define I2S_SOFT_RESET_RX_BIT	0x1
+#define I2S_SOFT_RESET_TX_BIT	0x2
+
+#define I2S_RX_IRQ		0x4c
+#define I2S_RX_IRQ_CONST	0x50
+#define I2S_TX_IRQ		0x54
+#define I2S_TX_IRQ_CONST	0x58
+
+#define I2S_IRQ_MASK	0x8
+#define I2S_IRQ_CLR	0xc
+#define I2S_IRQ_RX_BIT	0x1
+#define I2S_IRQ_TX_BIT	0x2
+
+#define I2S_MAX_BUFFER_SIZE	0x200000
+
+struct chv3_i2s_dev {
+	struct device *dev;
+	void __iomem *iobase;
+	void __iomem *iobase_irq;
+	struct snd_pcm_substream *rx_substream;
+	struct snd_pcm_substream *tx_substream;
+	int tx_bytes_to_fetch;
+	bool tx_ready;
+};
+
+static struct snd_soc_dai_driver chv3_i2s_dai = {
+	.name = "chv3-i2s",
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 128,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
+		.rate_min = 8000,
+		.rate_max = 96000,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.playback = {
+		.channels_min = 1,
+		.channels_max = 128,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
+		.rate_min = 8000,
+		.rate_max = 96000,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE,
+	},
+};
+
+static const struct snd_pcm_hardware chv3_dma_hw = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER,
+	.buffer_bytes_max = I2S_MAX_BUFFER_SIZE,
+	.period_bytes_min = 64,
+	.period_bytes_max = 8192,
+	.periods_min = 4,
+	.periods_max = 256,
+};
+
+static inline void chv3_i2s_wr(struct chv3_i2s_dev *i2s, int offset, u32 val)
+{
+	writel(val, i2s->iobase + offset);
+}
+
+static inline u32 chv3_i2s_rd(struct chv3_i2s_dev *i2s, int offset)
+{
+	return readl(i2s->iobase + offset);
+}
+
+static irqreturn_t chv3_i2s_isr(int irq, void *data)
+{
+	struct chv3_i2s_dev *i2s = data;
+	u32 reg;
+
+	reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
+	if (!reg)
+		return IRQ_NONE;
+
+	if (reg & I2S_IRQ_RX_BIT)
+		snd_pcm_period_elapsed(i2s->rx_substream);
+
+	if (reg & I2S_IRQ_TX_BIT) {
+		if (i2s->tx_ready)
+			snd_pcm_period_elapsed(i2s->tx_substream);
+		i2s->tx_ready = 1;
+	}
+
+	writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
+
+	return IRQ_HANDLED;
+}
+
+static int chv3_dma_open(struct snd_soc_component *component,
+			 struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+	int res;
+
+	snd_soc_set_runtime_hwparams(substream, &chv3_dma_hw);
+
+	res = snd_pcm_hw_constraint_pow2(substream->runtime, 0,
+			SNDRV_PCM_HW_PARAM_BUFFER_BYTES);
+	if (res)
+		return res;
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		i2s->rx_substream = substream;
+	else
+		i2s->tx_substream = substream;
+
+	return 0;
+}
+static int chv3_dma_close(struct snd_soc_component *component,
+			  struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+
+	if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
+		chv3_i2s_wr(i2s, I2S_RX_ENABLE, 0);
+	else
+		chv3_i2s_wr(i2s, I2S_TX_ENABLE, 0);
+
+	return 0;
+}
+
+static int chv3_dma_pcm_construct(struct snd_soc_component *component,
+				  struct snd_soc_pcm_runtime *rtd)
+{
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+	struct snd_pcm_substream *substream;
+	int res;
+
+	substream = rtd->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
+	if (substream) {
+		res = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, i2s->dev,
+				I2S_MAX_BUFFER_SIZE, &substream->dma_buffer);
+		if (res)
+			return res;
+	}
+
+	substream = rtd->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;
+	if (substream) {
+		res = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, i2s->dev,
+				I2S_MAX_BUFFER_SIZE, &substream->dma_buffer);
+		if (res)
+			return res;
+	}
+
+	return 0;
+}
+
+static int chv3_dma_hw_params(struct snd_soc_component *component,
+			      struct snd_pcm_substream *substream,
+			      struct snd_pcm_hw_params *params)
+{
+	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
+	return 0;
+}
+
+static int chv3_dma_prepare(struct snd_soc_component *component,
+			    struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+	unsigned int buffer_bytes, period_bytes, period_size;
+
+	buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
+	period_bytes = snd_pcm_lib_period_bytes(substream);
+	period_size = substream->runtime->period_size;
+
+	if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		chv3_i2s_wr(i2s, I2S_SOFT_RESET, I2S_SOFT_RESET_RX_BIT);
+		chv3_i2s_wr(i2s, I2S_RX_BASE_ADDR, substream->dma_buffer.addr);
+		chv3_i2s_wr(i2s, I2S_RX_BUFFER_SIZE, buffer_bytes);
+		chv3_i2s_wr(i2s, I2S_RX_IRQ, (period_size << 8) | 1);
+		chv3_i2s_wr(i2s, I2S_RX_ENABLE, 1);
+	} else {
+		i2s->tx_ready = 0;
+		chv3_i2s_wr(i2s, I2S_SOFT_RESET, I2S_SOFT_RESET_TX_BIT);
+		chv3_i2s_wr(i2s, I2S_TX_BASE_ADDR, substream->dma_buffer.addr);
+		chv3_i2s_wr(i2s, I2S_TX_BUFFER_SIZE, buffer_bytes);
+		chv3_i2s_wr(i2s, I2S_TX_IRQ, ((period_bytes / i2s->tx_bytes_to_fetch) << 8) | 1);
+		chv3_i2s_wr(i2s, I2S_TX_ENABLE, 1);
+	}
+	writel(I2S_IRQ_RX_BIT | I2S_IRQ_TX_BIT, i2s->iobase_irq + I2S_IRQ_MASK);
+
+	return 0;
+}
+
+static snd_pcm_uframes_t chv3_dma_pointer(struct snd_soc_component *component,
+					  struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+	u32 period_bytes, buffer_bytes;
+	u32 idx_bytes;
+
+	period_bytes = snd_pcm_lib_period_bytes(substream);
+	buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
+
+	if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		idx_bytes = chv3_i2s_rd(i2s, I2S_RX_PRODUCER_IDX);
+	} else {
+		idx_bytes = chv3_i2s_rd(i2s, I2S_TX_CONSUMER_IDX);
+		if (i2s->tx_ready)
+			idx_bytes = (idx_bytes - period_bytes) & (buffer_bytes - 1);
+		else
+			idx_bytes = 0;
+	}
+
+	return bytes_to_frames(substream->runtime, idx_bytes);
+}
+
+static int chv3_dma_ack(struct snd_soc_component *component,
+			struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct chv3_i2s_dev *i2s = snd_soc_dai_get_drvdata(asoc_rtd_to_cpu(rtd, 0));
+	unsigned int bytes, idx;
+
+	bytes = frames_to_bytes(runtime, runtime->control->appl_ptr);
+	idx = bytes & (snd_pcm_lib_buffer_bytes(substream) - 1);
+
+	if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
+		chv3_i2s_wr(i2s, I2S_RX_CONSUMER_IDX, idx);
+	else
+		chv3_i2s_wr(i2s, I2S_TX_PRODUCER_IDX, idx);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver chv3_i2s_comp = {
+	.name = "chv3-i2s-comp",
+	.open = chv3_dma_open,
+	.close = chv3_dma_close,
+	.pcm_construct = chv3_dma_pcm_construct,
+	.hw_params = chv3_dma_hw_params,
+	.prepare = chv3_dma_prepare,
+	.pointer = chv3_dma_pointer,
+	.ack = chv3_dma_ack,
+};
+
+static int chv3_i2s_probe(struct platform_device *pdev)
+{
+	struct chv3_i2s_dev *i2s;
+	int res;
+	int irq;
+
+	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
+	if (!i2s)
+		return -ENOMEM;
+
+	i2s->iobase = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(i2s->iobase))
+		return PTR_ERR(i2s->iobase);
+
+	i2s->iobase_irq = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(i2s->iobase_irq))
+		return PTR_ERR(i2s->iobase_irq);
+
+	i2s->tx_bytes_to_fetch = (chv3_i2s_rd(i2s, I2S_TX_IRQ_CONST) >> 8) & 0xffff;
+
+	i2s->dev = &pdev->dev;
+	dev_set_drvdata(&pdev->dev, i2s);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -ENXIO;
+	res = devm_request_irq(i2s->dev, irq, chv3_i2s_isr, 0, "chv3-i2s", i2s);
+	if (res)
+		return res;
+
+	res = devm_snd_soc_register_component(&pdev->dev, &chv3_i2s_comp,
+					      &chv3_i2s_dai, 1);
+	if (res) {
+		dev_err(&pdev->dev, "couldn't register component: %d\n", res);
+		return res;
+	}
+
+	dev_info(&pdev->dev, "probed\n");
+
+	return 0;
+}
+
+static const struct of_device_id chv3_i2s_of_match[] = {
+	{ .compatible = "google,chv3-i2s" },
+	{},
+};
+
+static struct platform_driver chv3_i2s_driver = {
+	.probe = chv3_i2s_probe,
+	.driver = {
+		.name = "chv3-i2s",
+		.of_match_table = chv3_i2s_of_match,
+	},
+};
+
+module_platform_driver(chv3_i2s_driver);
+
+MODULE_AUTHOR("Pawel Anikiel <pan@semihalf.com>");
+MODULE_DESCRIPTION("Chameleon v3 I2S interface");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/chameleonv3/chv3-it68051.c b/sound/soc/chameleonv3/chv3-it68051.c
new file mode 100644
index 000000000000..4813e699ec9c
--- /dev/null
+++ b/sound/soc/chameleonv3/chv3-it68051.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+#include <sound/soc.h>
+
+static struct snd_soc_dai_driver it68051_dai = {
+	.name = "it68051-hifi",
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 8,
+		.channels_max = 8,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE,
+	},
+};
+
+static const struct snd_soc_component_driver soc_component_dev_it68051 = {
+};
+
+static int it68051_probe(struct platform_device *pdev)
+{
+	return devm_snd_soc_register_component(&pdev->dev,
+		&soc_component_dev_it68051, &it68051_dai, 1);
+}
+
+static const struct of_device_id it68051_of_match[] = {
+	{ .compatible = "ite,it68051", },
+	{ }
+};
+
+static struct platform_driver it68051_platform_driver = {
+	.driver = {
+		.name = "it68051",
+		.of_match_table = it68051_of_match,
+	},
+	.probe = it68051_probe,
+};
+module_platform_driver(it68051_platform_driver);
+
+MODULE_DESCRIPTION("ASoC IT68051 driver");
+MODULE_AUTHOR("Pawel Anikiel <pan@semihalf.com>");
+MODULE_LICENSE("GPL");
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Krzysztof Kozlowski]

On 14/04/2023 16:01, Paweł Anikiel wrote:
> Add machine and platform drivers for ASoC audio on Chameleon v3.
> 
> The board has two audio sources: HDMI audio from the it68051 chip
> (RX only), and analog audio from the ssm2603 chip (RX and TX).
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../boot/dts/socfpga_arria10_chameleonv3.dts  |  28 ++

No way. DTS is always, always separate.

Best regards,
Krzysztof

Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Mark Brown]

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

On Fri, Apr 14, 2023 at 04:01:55PM +0200, Paweł Anikiel wrote:

> ---
>  .../boot/dts/socfpga_arria10_chameleonv3.dts  |  28 ++

Updates to the DT should be in a separate patch.

>  sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
>  sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
>  sound/soc/chameleonv3/chv3-it68051.c          |  41 +++

The machine driver and board drivers (if needed) should also be separate
patches - one patch per driver.

> +config SND_SOC_CHV3
> +       tristate "SoC Audio support for Chameleon v3"
> +       select SND_SOC_SSM2602
> +       select SND_SOC_SSM2602_I2C
> +       help
> +         Say Y if you want to add audio support for the Chameleon v3.

It woudl be better to have a separate selectable symbol for each drier.

> +static int chv3_ssm2603_hw_params(struct snd_pcm_substream *substream,
> +			  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
> +
> +	return snd_soc_dai_set_sysclk(dai, 0, 22579200, SND_SOC_CLOCK_IN);
> +}

This could be done once at init, though in general I can't tell why this
isn't audio-graph-card.

> + * Because of the two pointer design, the ring buffer can never be full. With
> + * capture this isn't a problem, because the hardware being the producer
> + * will wait for the consumer index to move out of the way.  With playback,
> + * however, this is problematic, because ALSA wants to fill up the buffer
> + * completely when waiting for hardware. In the .ack callback, the driver
> + * would have to wait for the consumer index to move out of the way by
> + * busy-waiting, which would keep stalling the kernel for quite a long time.
> + *
> + * The workaround to this problem is to "lie" to ALSA that the hw_pointer
> + * is one period behind what it actually is (see chv3_dma_pointer). This
> + * way, ALSA will not try to fill up the entire buffer, and all callbacks
> + * are wait-free.

Would it not be better to just lag by one (or some small number of)
sample instead?

> +static irqreturn_t chv3_i2s_isr(int irq, void *data)
> +{
> +	struct chv3_i2s_dev *i2s = data;
> +	u32 reg;
> +
> +	reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
> +	if (!reg)
> +		return IRQ_NONE;
> +
> +	if (reg & I2S_IRQ_RX_BIT)
> +		snd_pcm_period_elapsed(i2s->rx_substream);
> +
> +	if (reg & I2S_IRQ_TX_BIT) {
> +		if (i2s->tx_ready)
> +			snd_pcm_period_elapsed(i2s->tx_substream);
> +		i2s->tx_ready = 1;
> +	}
> +
> +	writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
> +
> +	return IRQ_HANDLED;
> +}

Really we should only ack things that were handled here and report
appropriately, that's defensive against bugs causing interrupts to
scream and shared interrupts.

> +	dev_info(&pdev->dev, "probed\n");

This is just noise, remove it.

> +++ b/sound/soc/chameleonv3/chv3-it68051.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/module.h>
> +#include <sound/soc.h>
> +
> +static struct snd_soc_dai_driver it68051_dai = {
> +	.name = "it68051-hifi",
> +	.capture = {
> +		.stream_name = "Capture",
> +		.channels_min = 8,
> +		.channels_max = 8,
> +		.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +		.formats = SNDRV_PCM_FMTBIT_S32_LE,
> +	},
> +};
> +
> +static const struct snd_soc_component_driver soc_component_dev_it68051 = {
> +};

This looks awfully like it's a generic CODEC driver for a device with no
control available, why is it not being added as a CODEC?

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

Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Paweł Anikiel]

Thank you for all the comments, I'm sorry for taking long to respond.

On Fri, Apr 14, 2023 at 7:19 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Apr 14, 2023 at 04:01:55PM +0200, Paweł Anikiel wrote:
>
> > ---
> >  .../boot/dts/socfpga_arria10_chameleonv3.dts  |  28 ++
>
> Updates to the DT should be in a separate patch.

Ok, I'll fix in next patchset

>
> >  sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
> >  sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
> >  sound/soc/chameleonv3/chv3-it68051.c          |  41 +++
>
> The machine driver and board drivers (if needed) should also be separate
> patches - one patch per driver.

Ok, I'll fix in next patchset

>
> > +config SND_SOC_CHV3
> > +       tristate "SoC Audio support for Chameleon v3"
> > +       select SND_SOC_SSM2602
> > +       select SND_SOC_SSM2602_I2C
> > +       help
> > +         Say Y if you want to add audio support for the Chameleon v3.
>
> It woudl be better to have a separate selectable symbol for each drier.

I'm not sure about this. If I disable just one driver, the entire card
fails to probe (even if some audio device doesn't need that driver).
Does it then make sense to be able to deselect some drivers? Please
correct me if I'm misunderstanding.

>
> > +static int chv3_ssm2603_hw_params(struct snd_pcm_substream *substream,
> > +                       struct snd_pcm_hw_params *params)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> > +     struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
> > +
> > +     return snd_soc_dai_set_sysclk(dai, 0, 22579200, SND_SOC_CLOCK_IN);
> > +}
>
> This could be done once at init, though in general I can't tell why this
> isn't audio-graph-card.

I will read up on audio-graph-card and see if it's possible to use here

>
> > + * Because of the two pointer design, the ring buffer can never be full. With
> > + * capture this isn't a problem, because the hardware being the producer
> > + * will wait for the consumer index to move out of the way.  With playback,
> > + * however, this is problematic, because ALSA wants to fill up the buffer
> > + * completely when waiting for hardware. In the .ack callback, the driver
> > + * would have to wait for the consumer index to move out of the way by
> > + * busy-waiting, which would keep stalling the kernel for quite a long time.
> > + *
> > + * The workaround to this problem is to "lie" to ALSA that the hw_pointer
> > + * is one period behind what it actually is (see chv3_dma_pointer). This
> > + * way, ALSA will not try to fill up the entire buffer, and all callbacks
> > + * are wait-free.
>
> Would it not be better to just lag by one (or some small number of)
> sample instead?

The benefit of lagging by an entire period is that we get an IRQ when
the first period is processed, which means we can drop the first call
to snd_pcm_period_elapsed() and have everything be coherent.

Having said that, I did try to remove that logic and simply delay
hw_pointer by one frame, and it appears to work (the playback seems
fine and without glitches). However, I'm worried about calling
snd_pcm_period_elapsed() and then reporting that the hw_pointer hasn't
actually reached the end of the period. Is that ok to do?

>
> > +static irqreturn_t chv3_i2s_isr(int irq, void *data)
> > +{
> > +     struct chv3_i2s_dev *i2s = data;
> > +     u32 reg;
> > +
> > +     reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
> > +     if (!reg)
> > +             return IRQ_NONE;
> > +
> > +     if (reg & I2S_IRQ_RX_BIT)
> > +             snd_pcm_period_elapsed(i2s->rx_substream);
> > +
> > +     if (reg & I2S_IRQ_TX_BIT) {
> > +             if (i2s->tx_ready)
> > +                     snd_pcm_period_elapsed(i2s->tx_substream);
> > +             i2s->tx_ready = 1;
> > +     }
> > +
> > +     writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> Really we should only ack things that were handled here and report
> appropriately, that's defensive against bugs causing interrupts to
> scream and shared interrupts.

What do you mean by handled? Should I check the hardware pointer and
check if a period really has elapsed?

>
> > +     dev_info(&pdev->dev, "probed\n");
>
> This is just noise, remove it.

Ok

>
> > +++ b/sound/soc/chameleonv3/chv3-it68051.c
> > @@ -0,0 +1,41 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/module.h>
> > +#include <sound/soc.h>
> > +
> > +static struct snd_soc_dai_driver it68051_dai = {
> > +     .name = "it68051-hifi",
> > +     .capture = {
> > +             .stream_name = "Capture",
> > +             .channels_min = 8,
> > +             .channels_max = 8,
> > +             .rates = SNDRV_PCM_RATE_CONTINUOUS,
> > +             .formats = SNDRV_PCM_FMTBIT_S32_LE,
> > +     },
> > +};
> > +
> > +static const struct snd_soc_component_driver soc_component_dev_it68051 = {
> > +};
>
> This looks awfully like it's a generic CODEC driver for a device with no
> control available,

Yes, it's a dummy capture-only codec with fixed 8 channels, arbitrary
rate, and S32_LE format.

> why is it not being added as a CODEC?

Do you mean I should put it in sound/soc/codecs/?

Also, I used the name of the HDMI receiver chip (IT68051), but really
this goes through some extra processing in an FPGA, so the result has
little in common with the chip itself. Do you have any advice on how
it should be named?

Regards,
Paweł

Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Mark Brown]

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

On Tue, Apr 25, 2023 at 05:59:52PM +0200, Paweł Anikiel wrote:

> > > +config SND_SOC_CHV3
> > > +       tristate "SoC Audio support for Chameleon v3"
> > > +       select SND_SOC_SSM2602
> > > +       select SND_SOC_SSM2602_I2C
> > > +       help
> > > +         Say Y if you want to add audio support for the Chameleon v3.

> > It woudl be better to have a separate selectable symbol for each drier.

> I'm not sure about this. If I disable just one driver, the entire card
> fails to probe (even if some audio device doesn't need that driver).
> Does it then make sense to be able to deselect some drivers? Please
> correct me if I'm misunderstanding.

Consider what happens if someone for example reuses the I2S controller
in a different board.

> Having said that, I did try to remove that logic and simply delay
> hw_pointer by one frame, and it appears to work (the playback seems
> fine and without glitches). However, I'm worried about calling
> snd_pcm_period_elapsed() and then reporting that the hw_pointer hasn't
> actually reached the end of the period. Is that ok to do?

It should be fine, things should be working off the hw_pointer.

> > > +     reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
> > > +     if (!reg)
> > > +             return IRQ_NONE;

> > > +     if (reg & I2S_IRQ_RX_BIT)
> > > +
> > > +     if (reg & I2S_IRQ_TX_BIT) {

> > > +     writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}

> > Really we should only ack things that were handled here and report
> > appropriately, that's defensive against bugs causing interrupts to
> > scream and shared interrupts.

> What do you mean by handled? Should I check the hardware pointer and
> check if a period really has elapsed?

The driver is checking for specific bits in the status register but
blindly acknowledging all bits that are set, and reporting IRQ_HANDLED
even if none are set.

> > why is it not being added as a CODEC?

> Do you mean I should put it in sound/soc/codecs/?

Yes.

> Also, I used the name of the HDMI receiver chip (IT68051), but really
> this goes through some extra processing in an FPGA, so the result has
> little in common with the chip itself. Do you have any advice on how
> it should be named?

If it's genuinely unrelated to the capabilities of the actual chip then
just putting a standalone driver in the platform directory is fine, but
the code should be clear about that otherwise it looks like the code for
the device could be shared.

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

[PATCH 2/9] dt-bindings: ASoC: Add chv3-i2s

[Paweł Anikiel]

Add binding for chv3-i2s device.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 .../bindings/sound/google,chv3-i2s.yaml       | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml

diff --git a/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
new file mode 100644
index 000000000000..6f49cf059ac5
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/google,chv3-i2s.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Google Chameleon v3 I2S device
+
+maintainers:
+  - Paweł Anikiel <pan@semihalf.com>
+
+description: |
+  I2S device for the Google Chameleon v3. The device handles both RX
+  and TX using a producer/consumer ring buffer design.
+
+properties:
+  compatible:
+    const: google,chv3-i2s
+  reg:
+    items:
+      - description: core registers
+      - description: irq registers
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2s0: i2s@c0060300 {
+        compatible = "google,chv3-i2s";
+        reg = <0xc0060300 0x100>,
+              <0xc0060f00 0x10>;
+        interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;
+    };
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 2/9] dt-bindings: ASoC: Add chv3-i2s

[Krzysztof Kozlowski]

On 14/04/2023 16:01, Paweł Anikiel wrote:
> Add binding for chv3-i2s device.

Your subject needs improvements:
1. ASoC goes before bindings
2. You miss some meaningful title of device. "chv3-i2s" can be anything,
so add Google or Google Chameleon. Or use entire compatible.


> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../bindings/sound/google,chv3-i2s.yaml       | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> new file mode 100644
> index 000000000000..6f49cf059ac5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/google,chv3-i2s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chameleon v3 I2S device
> +
> +maintainers:
> +  - Paweł Anikiel <pan@semihalf.com>
> +
> +description: |
> +  I2S device for the Google Chameleon v3. The device handles both RX
> +  and TX using a producer/consumer ring buffer design.
> +
> +properties:
> +  compatible:
> +    const: google,chv3-i2s

Missing blank line.

Is chv3 the name of your SoC? Where are the SoC bindings? What's exactly
the versioning scheme for it (compatibles must be specific, not generic).

> +  reg:
> +    items:
> +      - description: core registers
> +      - description: irq registers

As well...

> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2s0: i2s@c0060300 {
> +        compatible = "google,chv3-i2s";
> +        reg = <0xc0060300 0x100>,
> +              <0xc0060f00 0x10>;
> +        interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;

Isn't 0 also a known define?



Best regards,
Krzysztof

Re: [PATCH 2/9] dt-bindings: ASoC: Add chv3-i2s

[Paweł Anikiel]

On Fri, Apr 14, 2023 at 7:00 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/04/2023 16:01, Paweł Anikiel wrote:
> > Add binding for chv3-i2s device.
>
> Your subject needs improvements:
> 1. ASoC goes before bindings
> 2. You miss some meaningful title of device. "chv3-i2s" can be anything,
> so add Google or Google Chameleon. Or use entire compatible.

Would "ASoC: dt-bindings: Add Google Chameleon v3 I2S device" be better?

>
>
> >
> > Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> > ---
> >  .../bindings/sound/google,chv3-i2s.yaml       | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> > new file mode 100644
> > index 000000000000..6f49cf059ac5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/google,chv3-i2s.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Google Chameleon v3 I2S device
> > +
> > +maintainers:
> > +  - Paweł Anikiel <pan@semihalf.com>
> > +
> > +description: |
> > +  I2S device for the Google Chameleon v3. The device handles both RX
> > +  and TX using a producer/consumer ring buffer design.
> > +
> > +properties:
> > +  compatible:
> > +    const: google,chv3-i2s
>
> Missing blank line.
>
> Is chv3 the name of your SoC? Where are the SoC bindings? What's exactly
> the versioning scheme for it (compatibles must be specific, not generic).

The Chameleon v3 is based around an Intel Arria 10 SoC FPGA. The i2s
device is implemented inside the FPGA. Does this case require SoC
bindings?

>
> > +  reg:
> > +    items:
> > +      - description: core registers
> > +      - description: irq registers
>
> As well...
>
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2s0: i2s@c0060300 {
> > +        compatible = "google,chv3-i2s";
> > +        reg = <0xc0060300 0x100>,
> > +              <0xc0060f00 0x10>;
> > +        interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;
>
> Isn't 0 also a known define?

Do you mean this?
interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;

Regards,
Paweł

Re: [PATCH 2/9] dt-bindings: ASoC: Add chv3-i2s

[Krzysztof Kozlowski]

On 25/04/2023 18:01, Paweł Anikiel wrote:
> On Fri, Apr 14, 2023 at 7:00 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/04/2023 16:01, Paweł Anikiel wrote:
>>> Add binding for chv3-i2s device.
>>
>> Your subject needs improvements:
>> 1. ASoC goes before bindings
>> 2. You miss some meaningful title of device. "chv3-i2s" can be anything,
>> so add Google or Google Chameleon. Or use entire compatible.
> 
> Would "ASoC: dt-bindings: Add Google Chameleon v3 I2S device" be better?

Yes, thanks.

> 
>>
>>
>>>
>>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
>>> ---
>>>  .../bindings/sound/google,chv3-i2s.yaml       | 42 +++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>> new file mode 100644
>>> index 000000000000..6f49cf059ac5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>> @@ -0,0 +1,42 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sound/google,chv3-i2s.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Google Chameleon v3 I2S device
>>> +
>>> +maintainers:
>>> +  - Paweł Anikiel <pan@semihalf.com>
>>> +
>>> +description: |
>>> +  I2S device for the Google Chameleon v3. The device handles both RX
>>> +  and TX using a producer/consumer ring buffer design.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: google,chv3-i2s
>>
>> Missing blank line.
>>
>> Is chv3 the name of your SoC? Where are the SoC bindings? What's exactly
>> the versioning scheme for it (compatibles must be specific, not generic).
> 
> The Chameleon v3 is based around an Intel Arria 10 SoC FPGA. The i2s
> device is implemented inside the FPGA. Does this case require SoC
> bindings?

No, I was mistaken. I somehow get impression that's for Pixel... Sorry
for the noise.

> 
>>
>>> +  reg:
>>> +    items:
>>> +      - description: core registers
>>> +      - description: irq registers
>>
>> As well...
>>
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +
>>> +    i2s0: i2s@c0060300 {
>>> +        compatible = "google,chv3-i2s";
>>> +        reg = <0xc0060300 0x100>,
>>> +              <0xc0060f00 0x10>;
>>> +        interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;
>>
>> Isn't 0 also a known define?
> 
> Do you mean this?
> interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;


Yes, please.

Best regards,
Krzysztof

[PATCH 3/9] dt-bindings: ASoC: Add chv3-audio

[Paweł Anikiel]

Add binding for chv3-audio device.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 .../bindings/sound/google,chv3-audio.yaml     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-audio.yaml

diff --git a/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
new file mode 100644
index 000000000000..8b602b60eaee
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/google,chv3-audio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Google Chameleon v3 audio
+
+maintainers:
+  - Paweł Anikiel <pan@semihalf.com>
+
+properties:
+  compatible:
+    const: google,chv3-audio
+
+  google,audio-cpu0:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of controller #0
+
+  google,audio-codec0:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of codec #0
+
+  google,audio-cpu1:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of controller #1
+
+  google,audio-codec1:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of codec #1
+
+required:
+  - compatible
+  - google,audio-cpu0:
+  - google,audio-codec0:
+  - google,audio-cpu1:
+  - google,audio-codec1:
+
+additionalProperties: false
+
+examples:
+  - |
+    sound {
+        compatible = "google,chv3-audio";
+        google,audio-cpu0 = <&i2s1>;
+        google,audio-codec0 = <&ssm2603>;
+        google,audio-cpu1 = <&i2s0>;
+        google,audio-codec1 = <&it68051>;
+    };
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 3/9] dt-bindings: ASoC: Add chv3-audio

[Rob Herring]

On Fri, 14 Apr 2023 16:01:57 +0200, Paweł Anikiel wrote:
> Add binding for chv3-audio device.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../bindings/sound/google,chv3-audio.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/sound/google,chv3-audio.yaml:34:23: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/sound/google,chv3-audio.yaml:35:25: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/sound/google,chv3-audio.yaml:36:23: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/sound/google,chv3-audio.yaml:37:25: [error] empty value in block mapping (empty-values)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml: required:1: {'google,audio-cpu0': None} is not of type 'string'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml: required:2: {'google,audio-codec0': None} is not of type 'string'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml: required:3: {'google,audio-cpu1': None} is not of type 'string'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml: required:4: {'google,audio-codec1': None} is not of type 'string'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml: ignoring, error in schema: required: 1
Documentation/devicetree/bindings/sound/google,chv3-audio.example.dtb: /example-0/sound: failed to match any schema with compatible: ['google,chv3-audio']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230414140203.707729-4-pan@semihalf.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 3/9] dt-bindings: ASoC: Add chv3-audio

[Rob Herring]

On Fri, Apr 14, 2023 at 04:01:57PM +0200, Paweł Anikiel wrote:
> Add binding for chv3-audio device.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../bindings/sound/google,chv3-audio.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> new file mode 100644
> index 000000000000..8b602b60eaee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/google,chv3-audio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chameleon v3 audio
> +
> +maintainers:
> +  - Paweł Anikiel <pan@semihalf.com>
> +
> +properties:
> +  compatible:
> +    const: google,chv3-audio
> +
> +  google,audio-cpu0:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of controller #0
> +
> +  google,audio-codec0:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of codec #0
> +
> +  google,audio-cpu1:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of controller #1
> +
> +  google,audio-codec1:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of codec #1

Why do you need custom properties when we have standard property to 
point to these things (sound-dai). We also have bindings to handle 
multiple devices.

Rob

Re: [PATCH 3/9] dt-bindings: ASoC: Add chv3-audio

[Paweł Anikiel]

On Mon, Apr 17, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Apr 14, 2023 at 04:01:57PM +0200, Paweł Anikiel wrote:
> > Add binding for chv3-audio device.
> >
> > Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> > ---
> >  .../bindings/sound/google,chv3-audio.yaml     | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> > new file mode 100644
> > index 000000000000..8b602b60eaee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/google,chv3-audio.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/google,chv3-audio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Google Chameleon v3 audio
> > +
> > +maintainers:
> > +  - Paweł Anikiel <pan@semihalf.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: google,chv3-audio
> > +
> > +  google,audio-cpu0:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of controller #0
> > +
> > +  google,audio-codec0:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of codec #0
> > +
> > +  google,audio-cpu1:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of controller #1
> > +
> > +  google,audio-codec1:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of codec #1
>
> Why do you need custom properties when we have standard property to
> point to these things (sound-dai). We also have bindings to handle
> multiple devices.

Thanks, I will read up on that and try to use it here

Regards,
Paweł

[PATCH 4/9] dt-bindings: ASoC: Add it68051

[Paweł Anikiel]

Add binding for it68051 device.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 .../bindings/sound/ite,it68051.yaml           | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/ite,it68051.yaml

diff --git a/Documentation/devicetree/bindings/sound/ite,it68051.yaml b/Documentation/devicetree/bindings/sound/ite,it68051.yaml
new file mode 100644
index 000000000000..341c171b8a8a
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ite,it68051.yaml
@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ite,it68051.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: it68051 audio codec
+
+maintainers:
+  - Paweł Anikiel <pan@semihalf.com>
+
+properties:
+  compatible:
+    const: ite,it68051
+
+required:
+  - compatible
+
+examples:
+  - |
+    it68051: audio-codec {
+        compatible = "ite,it68051";
+    };
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 4/9] dt-bindings: ASoC: Add it68051

[Krzysztof Kozlowski]

On 14/04/2023 16:01, Paweł Anikiel wrote:
> Add binding for it68051 device.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../bindings/sound/ite,it68051.yaml           | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/ite,it68051.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/ite,it68051.yaml b/Documentation/devicetree/bindings/sound/ite,it68051.yaml
> new file mode 100644
> index 000000000000..341c171b8a8a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/ite,it68051.yaml
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/ite,it68051.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: it68051 audio codec
> +
> +maintainers:
> +  - Paweł Anikiel <pan@semihalf.com>
> +
> +properties:
> +  compatible:
> +    const: ite,it68051
> +

No ref to common DAI properties? No sound-dai-cells? No resources?
Bindings are supposed to be complete.


> +required:
> +  - compatible
> +
> +examples:
> +  - |
> +    it68051: audio-codec {

Drop label, you do not use it here.

> +        compatible = "ite,it68051";
> +    };

Best regards,
Krzysztof

Re: [PATCH 4/9] dt-bindings: ASoC: Add it68051

[Rob Herring]

On Fri, 14 Apr 2023 16:01:58 +0200, Paweł Anikiel wrote:
> Add binding for it68051 device.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  .../bindings/sound/ite,it68051.yaml           | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/ite,it68051.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/ite,it68051.yaml: 'oneOf' conditional failed, one must be fixed:
	'unevaluatedProperties' is a required property
	'additionalProperties' is a required property
	hint: Either unevaluatedProperties or additionalProperties must be present
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230414140203.707729-5-pan@semihalf.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

[Paweł Anikiel]

Apply a workaround for what seems to be a hardware quirk: when using
an external MCLK signal, powering on Output and DAC for the first time
produces output distortions unless they're powered together with whole
chip power.

The workaround powers them on in probe for the first time, as doing it
later may be impossible (e.g. when starting playback while recording,
whole chip power will already be on).

Here are some initialization sequences run after all other control
registers were set (`ssmset reg val` sets the value of a register
via i2c):

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x07 # chip, out
  OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x87 # out, dac
  ssmset 0x06 0x07 # chip
  OK

  (disable MCLK)
  ssmset 0x09 0x01 # core
  ssmset 0x06 0x1f # chip
  ssmset 0x06 0x07 # out, dac
  (enable MCLK)
  OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x1f # chip
  ssmset 0x06 0x07 # out, dac
  NOT OK

  ssmset 0x06 0x1f # chip
  ssmset 0x09 0x01 # core
  ssmset 0x06 0x07 # out, dac
  NOT OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x0f # chip, out
  ssmset 0x06 0x07 # dac
  NOT OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x17 # chip, dac
  ssmset 0x06 0x07 # out
  NOT OK

Here are some sequences run at the very start before a sw reset (and
later using one of the NOT OK sequences from above):

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x07 # chip, out, dac
  OK

  (disable MCLK)
  ssmset 0x09 0x01 # core
  ssmset 0x06 0x07 # chip, out, dac
  (enable MCLK after reset)
  NOT OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x17 # chip, dac
  NOT OK

  ssmset 0x09 0x01 # core
  ssmset 0x06 0x0f # chip, out
  NOT OK

  ssmset 0x06 0x07 # chip, out, dac
  NOT OK

This was tested on a Google Chameleon v3 board using an SSM2603 with an
external MCLK. This doesn't seem to just be a PCB issue, as this was
also observed on a ZYBO Z7-10:
https://ez.analog.com/audio/f/q-a/543726/solved-ssm2603-right-output-offset-issue/480229

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 sound/soc/codecs/ssm2602.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index cbbe83b85ada..021e0c860fa1 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -589,6 +589,17 @@ static int ssm260x_component_probe(struct snd_soc_component *component)
 		return ret;
 	}
 
+	/* Workaround for what seems to be a hardware quirk: when using an
+	 * external MCLK signal, powering on Output and DAC for the first
+	 * time produces output distortions unless they're powered together
+	 * with whole chip power. We power them here for the first time,
+	 * as doing it later may be impossible (e.g. when starting playback
+	 * while recording, whole chip power will already be on)
+	 */
+	regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01);
+	regmap_write(ssm2602->regmap, SSM2602_PWR,    0x07);
+	regmap_write(ssm2602->regmap, SSM2602_RESET,  0x00);
+
 	/* set the update bits */
 	regmap_update_bits(ssm2602->regmap, SSM2602_LINVOL,
 			    LINVOL_LRIN_BOTH, LINVOL_LRIN_BOTH);
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

[Mark Brown]

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

On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote:

> Apply a workaround for what seems to be a hardware quirk: when using
> an external MCLK signal, powering on Output and DAC for the first time
> produces output distortions unless they're powered together with whole
> chip power.

This doesn't seem coherent, these are multiple register writes so
clearly can't be done at the same moment as initial power on.  Clearly
there's some other constraint here.

> The workaround powers them on in probe for the first time, as doing it
> later may be impossible (e.g. when starting playback while recording,
> whole chip power will already be on).

It doesn't do that, it powers them on at component probe.

> Here are some sequences run at the very start before a sw reset (and
> later using one of the NOT OK sequences from above):
> 
>   ssmset 0x09 0x01 # core
>   ssmset 0x06 0x07 # chip, out, dac
>   OK

I can't tell what any of this is trying to say, especially given all the
magic numbers, and obviously no actual use of the driver should be
writing directly to the register map.

> +	/* Workaround for what seems to be a hardware quirk: when using an
> +	 * external MCLK signal, powering on Output and DAC for the first
> +	 * time produces output distortions unless they're powered together
> +	 * with whole chip power. We power them here for the first time,
> +	 * as doing it later may be impossible (e.g. when starting playback
> +	 * while recording, whole chip power will already be on)
> +	 */
> +	regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01);
> +	regmap_write(ssm2602->regmap, SSM2602_PWR,    0x07);
> +	regmap_write(ssm2602->regmap, SSM2602_RESET,  0x00);
> +

The rest of the driver uses symbolic names for register values, this
code should too.  

This also seems buggy in that it writes non-default values to the
hardware then does a reset, meaning that the cache and hardware values
will be out of sync, and since it only happens on probe there will be an
issue after suspend if power is removed.  It looks like this would be
most comfortably implemented as a register patch applied as soon as the
regmap is instantiated.  See regmap_register_patch().

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

Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

[Paweł Anikiel]

On Fri, Apr 14, 2023 at 7:35 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote:
>
> > Apply a workaround for what seems to be a hardware quirk: when using
> > an external MCLK signal, powering on Output and DAC for the first time
> > produces output distortions unless they're powered together with whole
> > chip power.
>
> This doesn't seem coherent, these are multiple register writes so
> clearly can't be done at the same moment as initial power on.  Clearly
> there's some other constraint here.

The "at the same time" part is done by writing multiple bits at once
to SSM2602_PWR. But before that, SSM2602_ACTIVE has to be set, and
then the chip is reset (SSM2602_RESET) to power everything down again.

>
> > The workaround powers them on in probe for the first time, as doing it
> > later may be impossible (e.g. when starting playback while recording,
> > whole chip power will already be on).
>
> It doesn't do that, it powers them on at component probe.

Yes, I meant component probe.

>
> > Here are some sequences run at the very start before a sw reset (and
> > later using one of the NOT OK sequences from above):
> >
> >   ssmset 0x09 0x01 # core
> >   ssmset 0x06 0x07 # chip, out, dac
> >   OK
>
> I can't tell what any of this is trying to say, especially given all the
> magic numbers, and obviously no actual use of the driver should be
> writing directly to the register map.

These are shell commands run from userspace (with no ssm2602 driver
present in the kernel). ssmset is a wrapper for the i2cset command:
ssmset() {
        i2cset -y 0 0x1a $(($1*2)) $2
}
I definitely should have made that more clear.

Do you think these logs are worth adding? If so, I'll improve the
explanation what these mean.

>
> > +     /* Workaround for what seems to be a hardware quirk: when using an
> > +      * external MCLK signal, powering on Output and DAC for the first
> > +      * time produces output distortions unless they're powered together
> > +      * with whole chip power. We power them here for the first time,
> > +      * as doing it later may be impossible (e.g. when starting playback
> > +      * while recording, whole chip power will already be on)
> > +      */
> > +     regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01);
> > +     regmap_write(ssm2602->regmap, SSM2602_PWR,    0x07);
> > +     regmap_write(ssm2602->regmap, SSM2602_RESET,  0x00);
> > +
>
> The rest of the driver uses symbolic names for register values, this
> code should too.

Ok, I'll correct that.

>
> This also seems buggy in that it writes non-default values to the
> hardware then does a reset, meaning that the cache and hardware values
> will be out of sync, and since it only happens on probe there will be an
> issue after suspend if power is removed.  It looks like this would be
> most comfortably implemented as a register patch applied as soon as the
> regmap is instantiated.  See regmap_register_patch().

I haven't considered that. I will look at regmap_register_patch() and
try to use it.

Regards,
Paweł

Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

[Mark Brown]

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

On Tue, Apr 25, 2023 at 06:02:20PM +0200, Paweł Anikiel wrote:
> On Fri, Apr 14, 2023 at 7:35 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote:

> > > Apply a workaround for what seems to be a hardware quirk: when using
> > > an external MCLK signal, powering on Output and DAC for the first time
> > > produces output distortions unless they're powered together with whole
> > > chip power.

> > This doesn't seem coherent, these are multiple register writes so
> > clearly can't be done at the same moment as initial power on.  Clearly
> > there's some other constraint here.

> The "at the same time" part is done by writing multiple bits at once
> to SSM2602_PWR. But before that, SSM2602_ACTIVE has to be set, and
> then the chip is reset (SSM2602_RESET) to power everything down again.

So you need to power up the chip then do a register write sequence -
that's noticably different to what the description says.

> > > Here are some sequences run at the very start before a sw reset (and
> > > later using one of the NOT OK sequences from above):

> > >   ssmset 0x09 0x01 # core
> > >   ssmset 0x06 0x07 # chip, out, dac
> > >   OK

> > I can't tell what any of this is trying to say, especially given all the
> > magic numbers, and obviously no actual use of the driver should be
> > writing directly to the register map.

> These are shell commands run from userspace (with no ssm2602 driver
> present in the kernel). ssmset is a wrapper for the i2cset command:
> ssmset() {
>         i2cset -y 0 0x1a $(($1*2)) $2
> }
> I definitely should have made that more clear.

> Do you think these logs are worth adding? If so, I'll improve the
> explanation what these mean.

Probably?  Since I couldn't really follow what these were trying to tell
me it's a bit hard to say.  It looks like you worked this out yourself
rather than using an erratum so documenting where the workaround comes
from seems useful.

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

[PATCH 6/9] ASoC: ssm2602: Add support for CLKDIV2

[Paweł Anikiel]

The SSM260x chips have an internal MCLK /2 divider (bit D7 in register
R8). Add logic that allows for more MCLK values using this divider.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 sound/soc/codecs/ssm2602.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index 021e0c860fa1..35c4743e756e 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -280,9 +280,12 @@ static inline int ssm2602_get_coeff(int mclk, int rate)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(ssm2602_coeff_table); i++) {
-		if (ssm2602_coeff_table[i].rate == rate &&
-			ssm2602_coeff_table[i].mclk == mclk)
-			return ssm2602_coeff_table[i].srate;
+		if (ssm2602_coeff_table[i].rate == rate) {
+			if (ssm2602_coeff_table[i].mclk == mclk)
+				return ssm2602_coeff_table[i].srate;
+			if (ssm2602_coeff_table[i].mclk == mclk / 2)
+				return ssm2602_coeff_table[i].srate | SRATE_CORECLK_DIV2;
+		}
 	}
 	return -EINVAL;
 }
@@ -365,18 +368,24 @@ static int ssm2602_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 		switch (freq) {
 		case 12288000:
 		case 18432000:
+		case 24576000:
+		case 36864000:
 			ssm2602->sysclk_constraints = &ssm2602_constraints_12288000;
 			break;
 		case 11289600:
 		case 16934400:
+		case 22579200:
+		case 33868800:
 			ssm2602->sysclk_constraints = &ssm2602_constraints_11289600;
 			break;
 		case 12000000:
+		case 24000000:
 			ssm2602->sysclk_constraints = NULL;
 			break;
 		default:
 			return -EINVAL;
 		}
+
 		ssm2602->sysclk = freq;
 	} else {
 		unsigned int mask;
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH 7/9] ASoC: ssm2602: Add mute gpio

[Paweł Anikiel]

The SSM2603 has a hardware mute pin. Add an optional mute gpio to
control it.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 sound/soc/codecs/ssm2602.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index 35c4743e756e..dd81e62d7a3b 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
 #include <linux/slab.h>
 
 #include <sound/pcm.h>
@@ -32,6 +33,8 @@ struct ssm2602_priv {
 
 	enum ssm2602_type type;
 	unsigned int clk_out_pwr;
+
+	struct gpio_desc *mute_gpiod;
 };
 
 /*
@@ -352,6 +355,10 @@ static int ssm2602_mute(struct snd_soc_dai *dai, int mute, int direction)
 	else
 		regmap_update_bits(ssm2602->regmap, SSM2602_APDIGI,
 				    APDIGI_ENABLE_DAC_MUTE, 0);
+
+	if (ssm2602->mute_gpiod)
+		gpiod_set_value_cansleep(ssm2602->mute_gpiod, mute);
+
 	return 0;
 }
 
@@ -680,6 +687,10 @@ int ssm2602_probe(struct device *dev, enum ssm2602_type type,
 	ssm2602->type = type;
 	ssm2602->regmap = regmap;
 
+	ssm2602->mute_gpiod = devm_gpiod_get_optional(dev, "mute", GPIOD_OUT_LOW);
+	if (IS_ERR(ssm2602->mute_gpiod))
+		return PTR_ERR(ssm2602->mute_gpiod);
+
 	return devm_snd_soc_register_component(dev, &soc_component_dev_ssm2602,
 		&ssm2602_dai, 1);
 }
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 7/9] ASoC: ssm2602: Add mute gpio

[Mark Brown]

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

On Fri, Apr 14, 2023 at 04:02:01PM +0200, Paweł Anikiel wrote:

> @@ -352,6 +355,10 @@ static int ssm2602_mute(struct snd_soc_dai *dai, int mute, int direction)
>  	else
>  		regmap_update_bits(ssm2602->regmap, SSM2602_APDIGI,
>  				    APDIGI_ENABLE_DAC_MUTE, 0);
> +
> +	if (ssm2602->mute_gpiod)
> +		gpiod_set_value_cansleep(ssm2602->mute_gpiod, mute);
> +

It seems pointless to control both the mute register and the GPIO mute
here, we gain nothing but overhead from having the GPIO.  What I would
suggest is that if the GPIO is present then that is used in _mute() and
we add a control allowing the user to mute and unmute via the register.

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

[PATCH 8/9] dt-bindings: ASoC: Add mute-gpio to ssm2602

[Paweł Anikiel]

Add mute-gpio to ssm2602

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 Documentation/devicetree/bindings/sound/adi,ssm2602.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
index 3b3302fe399b..132aa9668e7e 100644
--- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
+++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
@@ -11,6 +11,10 @@ Required properties:
   - reg : the I2C address of the device for I2C, the chip select
           number for SPI.
 
+Optional properties:
+
+- mute-gpios: GPIO specifier for the mute pin
+
  Example:
 
 	ssm2602: ssm2602@1a {
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH 9/9] ARM: dts: chameleonv3: Add ssm2603 mute gpio

[Paweł Anikiel]

The ssm2603 mute pin is connected to a gpio expander. Add this
connection to the devicetree.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
index c75f41058196..81e83ce0f8e7 100644
--- a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
+++ b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
@@ -3,6 +3,7 @@
  * Copyright 2022 Google LLC
  */
 /dts-v1/;
+#include <dt-bindings/gpio/gpio.h>
 #include "socfpga_arria10_mercury_aa1.dtsi"
 
 / {
@@ -67,6 +68,7 @@ &i2c0 {
 	ssm2603: audio-codec@1a {
 		compatible = "adi,ssm2603";
 		reg = <0x1a>;
+		mute-gpios = <&u80 0 GPIO_ACTIVE_LOW>;
 	};
 };
 
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH 0/9] Add Chameleon v3 ASoC audio

[Mark Brown]

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

On Fri, Apr 14, 2023 at 04:01:54PM +0200, Paweł Anikiel wrote:

>  sound/soc/Makefile                            |   1 +
>  sound/soc/chameleonv3/Kconfig                 |   7 +
>  sound/soc/chameleonv3/Makefile                |   2 +
>  sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
>  sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
>  sound/soc/chameleonv3/chv3-it68051.c          |  41 +++

Please at least make a directory for Google as a vendor, we don't want
people adding directories for each individual product.  That said
generally we add machine drivers in the directory for the relevant SoC
family, is there any reason that pattern isn't followed here?

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

Re: [PATCH 0/9] Add Chameleon v3 ASoC audio

[Paweł Anikiel]

On Fri, Apr 14, 2023 at 6:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Apr 14, 2023 at 04:01:54PM +0200, Paweł Anikiel wrote:
>
> >  sound/soc/Makefile                            |   1 +
> >  sound/soc/chameleonv3/Kconfig                 |   7 +
> >  sound/soc/chameleonv3/Makefile                |   2 +
> >  sound/soc/chameleonv3/chv3-audio.c            | 111 ++++++
> >  sound/soc/chameleonv3/chv3-i2s.c              | 347 ++++++++++++++++++
> >  sound/soc/chameleonv3/chv3-it68051.c          |  41 +++
>
> Please at least make a directory for Google as a vendor, we don't want
> people adding directories for each individual product. That said
> generally we add machine drivers in the directory for the relevant SoC
> family, is there any reason that pattern isn't followed here?

The board is based around an Intel Arria 10 SoC FPGA. The ring buffer
device and all the routing is implemented inside the FPGA. Is it ok to
put the machine driver in the product directory in this case?. As for
the directory path, would sound/soc/google/chameleonv3/* be ok?

Regards,
Paweł

Re: [PATCH 0/9] Add Chameleon v3 ASoC audio

[Mark Brown]

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

On Tue, Apr 25, 2023 at 05:58:59PM +0200, Paweł Anikiel wrote:

> > Please at least make a directory for Google as a vendor, we don't want
> > people adding directories for each individual product. That said
> > generally we add machine drivers in the directory for the relevant SoC
> > family, is there any reason that pattern isn't followed here?

> The board is based around an Intel Arria 10 SoC FPGA. The ring buffer
> device and all the routing is implemented inside the FPGA. Is it ok to
> put the machine driver in the product directory in this case?. As for
> the directory path, would sound/soc/google/chameleonv3/* be ok?

Does the individual product really need a directory - nobody's going to
reuse the IP on the FPGA for anything?

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

Re: (subset) [PATCH 0/9] Add Chameleon v3 ASoC audio

[Mark Brown]

On Fri, 14 Apr 2023 16:01:54 +0200, Paweł Anikiel wrote:
> The Google Chameleon v3 is a device made for testing audio and video
> paths of other devices. This patchset adds support for ASoC audio on
> this device. It has two audio sources: HDMI audio from the it68051 chip
> (RX only), and analog audio from the ssm2603 chip (RX and TX).
> 
> The patchset adds the ASoC platform and machine drivers, as well as some
> changes to the existing ssm2602 codec driver.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[6/9] ASoC: ssm2602: Add support for CLKDIV2
      commit: 8076c586bbc1c62e075e58f41dafdd8b5022b24d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark