[PATCH v4 00/11] ASoC: cleanups and improvements for jz4740-i2s

[PATCH v4 00/11] ASoC: cleanups and improvements for jz4740-i2s

[Aidan MacDonald]

This series is a preparatory cleanup of the jz4740-i2s driver before
adding support for a new SoC. The two improvements are lifting
unnecessary restrictions on sample rates and formats -- the existing
ones appear to be derived from the limitations of the JZ4740's internal
codec and don't reflect the actual capabilities of the I2S controller.

I'm unable to test the series on any JZ47xx SoCs, but I have tested
on an X1000 (which is the SoC I'll be adding in a followup series).

Changes in v2:

* Drop two patches already in sound for-next.
* Squash two removal patches into the regmap fields patch.
* Remove the unused 'mem' resource in the driver private struct.
* Use regmap_set_bits() and regmap_clear_bits() to improve readability.
* Add fix for SoCs with independent FIFO flush bits (ie. most of them).
* Update sample formats patch with a more informative commit message.
* Add two new patches to refactor DAI/component probing.

Changes in v3:

* Fix missing 'ret' in patch 11 (yes, that was pretty silly of me)

Changes in v4:

* Refactor FIFO flush bits fix so it doesn't depend on regmap conversion.

Aidan MacDonald (11):
  ASoC: jz4740-i2s: Handle independent FIFO flush bits
  ASoC: jz4740-i2s: Remove unused 'mem' resource
  ASoC: jz4740-i2s: Convert to regmap API
  ASoC: jz4740-i2s: Simplify using regmap fields
  ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback
  ASoC: jz4740-i2s: Align macro values and sort includes
  ASoC: jz4740-i2s: Make the PLL clock name SoC-specific
  ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats
  ASoC: jz4740-i2s: Support continuous sample rate
  ASoC: jz4740-i2s: Move component functions near the component driver
  ASoC: jz4740-i2s: Refactor DAI probe/remove ops as component ops

 sound/soc/jz4740/Kconfig      |   1 +
 sound/soc/jz4740/jz4740-i2s.c | 461 ++++++++++++++++++----------------
 2 files changed, 248 insertions(+), 214 deletions(-)

-- 
2.35.1

[PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits

[Aidan MacDonald]

On the JZ4740, there is a single bit that flushes (empties) both
the transmit and receive FIFO. Later SoCs have independent flush
bits for each FIFO, which allows us to flush the right FIFO when
starting up a stream.

This also fixes a bug: since we were only setting the JZ4740's
flush bit, which corresponds to the TX FIFO flush bit on other
SoCs, other SoCs were not having their RX FIFO flushed at all.

Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index ecd8df70d39c..576f31f9d734 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -64,6 +64,9 @@
 #define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1)
 #define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0)
 
+#define JZ4760_AIC_CTRL_TFLUSH BIT(8)
+#define JZ4760_AIC_CTRL_RFLUSH BIT(7)
+
 #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
 #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
 
@@ -90,6 +93,8 @@ enum jz47xx_i2s_version {
 struct i2s_soc_info {
 	enum jz47xx_i2s_version version;
 	struct snd_soc_dai_driver *dai;
+
+	bool shared_fifo_flush;
 };
 
 struct jz4740_i2s {
@@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
 	uint32_t conf, ctrl;
 	int ret;
 
+	/*
+	 * When we can flush FIFOs independently, only flush the
+	 * FIFO that is starting up.
+	 */
+	if (!i2s->soc_info->shared_fifo_flush) {
+		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
+		else
+			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
+
+		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
+	}
+
 	if (snd_soc_dai_active(dai))
 		return 0;
 
-	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
-	ctrl |= JZ_AIC_CTRL_FLUSH;
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
+	/*
+	 * When there is a shared flush bit for both FIFOs we can
+	 * only flush the FIFOs if no other stream has started.
+	 */
+	if (i2s->soc_info->shared_fifo_flush) {
+		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
+		ctrl |= JZ_AIC_CTRL_FLUSH;
+		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
+	}
 
 	ret = clk_prepare_enable(i2s->clk_i2s);
 	if (ret)
@@ -444,6 +470,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = {
 static const struct i2s_soc_info jz4740_i2s_soc_info = {
 	.version = JZ_I2S_JZ4740,
 	.dai = &jz4740_i2s_dai,
+	.shared_fifo_flush = true,
 };
 
 static const struct i2s_soc_info jz4760_i2s_soc_info = {
-- 
2.35.1

[PATCH v4 02/11] ASoC: jz4740-i2s: Remove unused 'mem' resource

[Aidan MacDonald]

This isn't used and doesn't need to be in the private data struct.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 576f31f9d734..adf896333584 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -98,7 +98,6 @@ struct i2s_soc_info {
 };
 
 struct jz4740_i2s {
-	struct resource *mem;
 	void __iomem *base;
 
 	struct clk *clk_aic;
-- 
2.35.1

[PATCH v4 03/11] ASoC: jz4740-i2s: Convert to regmap API

[Aidan MacDonald]

Using regmap for accessing the AIC registers makes the driver a
little easier to read, and later refactors can take advantage of
regmap APIs to further simplify the driver.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/Kconfig      |   1 +
 sound/soc/jz4740/jz4740-i2s.c | 107 +++++++++++++---------------------
 2 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/sound/soc/jz4740/Kconfig b/sound/soc/jz4740/Kconfig
index e72f826062e9..dd3b4507fbe6 100644
--- a/sound/soc/jz4740/Kconfig
+++ b/sound/soc/jz4740/Kconfig
@@ -3,6 +3,7 @@ config SND_JZ4740_SOC_I2S
 	tristate "SoC Audio (I2S protocol) for Ingenic JZ4740 SoC"
 	depends on MIPS || COMPILE_TEST
 	depends on HAS_IOMEM
+	select REGMAP_MMIO
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 	help
 	  Say Y if you want to use I2S protocol and I2S codec on Ingenic JZ4740
diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index adf896333584..1393b383a886 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 #include <linux/clk.h>
@@ -98,7 +99,7 @@ struct i2s_soc_info {
 };
 
 struct jz4740_i2s {
-	void __iomem *base;
+	struct regmap *regmap;
 
 	struct clk *clk_aic;
 	struct clk *clk_i2s;
@@ -109,23 +110,10 @@ struct jz4740_i2s {
 	const struct i2s_soc_info *soc_info;
 };
 
-static inline uint32_t jz4740_i2s_read(const struct jz4740_i2s *i2s,
-	unsigned int reg)
-{
-	return readl(i2s->base + reg);
-}
-
-static inline void jz4740_i2s_write(const struct jz4740_i2s *i2s,
-	unsigned int reg, uint32_t value)
-{
-	writel(value, i2s->base + reg);
-}
-
 static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-	uint32_t conf, ctrl;
 	int ret;
 
 	/*
@@ -133,14 +121,10 @@ static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
 	 * FIFO that is starting up.
 	 */
 	if (!i2s->soc_info->shared_fifo_flush) {
-		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
-
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
+			regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, JZ4760_AIC_CTRL_TFLUSH);
 		else
-			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
-
-		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
+			regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, JZ4760_AIC_CTRL_RFLUSH);
 	}
 
 	if (snd_soc_dai_active(dai))
@@ -150,20 +134,14 @@ static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
 	 * When there is a shared flush bit for both FIFOs we can
 	 * only flush the FIFOs if no other stream has started.
 	 */
-	if (i2s->soc_info->shared_fifo_flush) {
-		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
-		ctrl |= JZ_AIC_CTRL_FLUSH;
-		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
-	}
+	if (i2s->soc_info->shared_fifo_flush)
+		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_FLUSH);
 
 	ret = clk_prepare_enable(i2s->clk_i2s);
 	if (ret)
 		return ret;
 
-	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
-	conf |= JZ_AIC_CONF_ENABLE;
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
-
+	regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
 	return 0;
 }
 
@@ -171,14 +149,11 @@ static void jz4740_i2s_shutdown(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-	uint32_t conf;
 
 	if (snd_soc_dai_active(dai))
 		return;
 
-	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
-	conf &= ~JZ_AIC_CONF_ENABLE;
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
+	regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
 
 	clk_disable_unprepare(i2s->clk_i2s);
 }
@@ -187,8 +162,6 @@ static int jz4740_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
-	uint32_t ctrl;
 	uint32_t mask;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
@@ -196,38 +169,30 @@ static int jz4740_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	else
 		mask = JZ_AIC_CTRL_ENABLE_CAPTURE | JZ_AIC_CTRL_ENABLE_RX_DMA;
 
-	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
-
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		ctrl |= mask;
+		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		ctrl &= ~mask;
+		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask);
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
-
 	return 0;
 }
 
 static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
-	uint32_t format = 0;
-	uint32_t conf;
-
-	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
-
-	conf &= ~(JZ_AIC_CONF_BIT_CLK_MASTER | JZ_AIC_CONF_SYNC_CLK_MASTER);
+	const unsigned int conf_mask = JZ_AIC_CONF_BIT_CLK_MASTER |
+				       JZ_AIC_CONF_SYNC_CLK_MASTER;
+	unsigned int conf = 0, format = 0;
 
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BP_FP:
@@ -263,8 +228,8 @@ static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return -EINVAL;
 	}
 
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
-	jz4740_i2s_write(i2s, JZ_REG_AIC_I2S_FMT, format);
+	regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF, conf_mask, conf);
+	regmap_write(i2s->regmap, JZ_REG_AIC_I2S_FMT, format);
 
 	return 0;
 }
@@ -277,9 +242,9 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 	uint32_t ctrl, div_reg;
 	int div;
 
-	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
+	regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
+	regmap_read(i2s->regmap, JZ_REG_AIC_CLK_DIV, &div_reg);
 
-	div_reg = jz4740_i2s_read(i2s, JZ_REG_AIC_CLK_DIV);
 	div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
 
 	switch (params_format(params)) {
@@ -316,8 +281,8 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 		}
 	}
 
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CLK_DIV, div_reg);
+	regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);
+	regmap_write(i2s->regmap, JZ_REG_AIC_CLK_DIV, div_reg);
 
 	return 0;
 }
@@ -354,13 +319,9 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 static int jz4740_i2s_suspend(struct snd_soc_component *component)
 {
 	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
-	uint32_t conf;
 
 	if (snd_soc_component_active(component)) {
-		conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
-		conf &= ~JZ_AIC_CONF_ENABLE;
-		jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
-
+		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
 		clk_disable_unprepare(i2s->clk_i2s);
 	}
 
@@ -372,7 +333,6 @@ static int jz4740_i2s_suspend(struct snd_soc_component *component)
 static int jz4740_i2s_resume(struct snd_soc_component *component)
 {
 	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
-	uint32_t conf;
 	int ret;
 
 	ret = clk_prepare_enable(i2s->clk_aic);
@@ -386,9 +346,7 @@ static int jz4740_i2s_resume(struct snd_soc_component *component)
 			return ret;
 		}
 
-		conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
-		conf |= JZ_AIC_CONF_ENABLE;
-		jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
+		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
 	}
 
 	return 0;
@@ -421,8 +379,8 @@ static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
 			JZ_AIC_CONF_INTERNAL_CODEC;
 	}
 
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
-	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
+	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
+	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, conf);
 
 	return 0;
 }
@@ -521,11 +479,19 @@ static const struct of_device_id jz4740_of_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, jz4740_of_matches);
 
+static const struct regmap_config jz4740_i2s_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= JZ_REG_AIC_FIFO,
+};
+
 static int jz4740_i2s_dev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct jz4740_i2s *i2s;
 	struct resource *mem;
+	void __iomem *regs;
 	int ret;
 
 	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
@@ -534,9 +500,9 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev)
 
 	i2s->soc_info = device_get_match_data(dev);
 
-	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
-	if (IS_ERR(i2s->base))
-		return PTR_ERR(i2s->base);
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
 
 	i2s->playback_dma_data.maxburst = 16;
 	i2s->playback_dma_data.addr = mem->start + JZ_REG_AIC_FIFO;
@@ -552,6 +518,11 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev)
 	if (IS_ERR(i2s->clk_i2s))
 		return PTR_ERR(i2s->clk_i2s);
 
+	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
+					    &jz4740_i2s_regmap_config);
+	if (IS_ERR(i2s->regmap))
+		return PTR_ERR(i2s->regmap);
+
 	platform_set_drvdata(pdev, i2s);
 
 	ret = devm_snd_soc_register_component(dev, &jz4740_i2s_component,
-- 
2.35.1

[PATCH v4 04/11] ASoC: jz4740-i2s: Simplify using regmap fields

[Aidan MacDonald]

The differences between register fields on different SoC versions
can be abstracted away using the regmap field API. This is easier
to understand and extend than comparisons based on the version ID.
Since the version IDs are unused after this change, remove them at
the same time, and remove unused macros.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 135 +++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 58 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 1393b383a886..043f100a9cfa 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -34,8 +34,6 @@
 #define JZ_REG_AIC_CLK_DIV	0x30
 #define JZ_REG_AIC_FIFO		0x34
 
-#define JZ_AIC_CONF_FIFO_RX_THRESHOLD_MASK (0xf << 12)
-#define JZ_AIC_CONF_FIFO_TX_THRESHOLD_MASK (0xf <<  8)
 #define JZ_AIC_CONF_OVERFLOW_PLAY_LAST BIT(6)
 #define JZ_AIC_CONF_INTERNAL_CODEC BIT(5)
 #define JZ_AIC_CONF_I2S BIT(4)
@@ -44,11 +42,6 @@
 #define JZ_AIC_CONF_SYNC_CLK_MASTER BIT(1)
 #define JZ_AIC_CONF_ENABLE BIT(0)
 
-#define JZ_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET 12
-#define JZ_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET 8
-#define JZ4760_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET 24
-#define JZ4760_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET 16
-
 #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK (0x7 << 19)
 #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK (0x7 << 16)
 #define JZ_AIC_CTRL_ENABLE_RX_DMA BIT(15)
@@ -78,29 +71,25 @@
 
 #define JZ_AIC_I2S_STATUS_BUSY BIT(2)
 
-#define JZ_AIC_CLK_DIV_MASK 0xf
-#define I2SDIV_DV_SHIFT 0
-#define I2SDIV_DV_MASK (0xf << I2SDIV_DV_SHIFT)
-#define I2SDIV_IDV_SHIFT 8
-#define I2SDIV_IDV_MASK (0xf << I2SDIV_IDV_SHIFT)
-
-enum jz47xx_i2s_version {
-	JZ_I2S_JZ4740,
-	JZ_I2S_JZ4760,
-	JZ_I2S_JZ4770,
-	JZ_I2S_JZ4780,
-};
-
 struct i2s_soc_info {
-	enum jz47xx_i2s_version version;
 	struct snd_soc_dai_driver *dai;
 
+	struct reg_field field_rx_fifo_thresh;
+	struct reg_field field_tx_fifo_thresh;
+	struct reg_field field_i2sdiv_capture;
+	struct reg_field field_i2sdiv_playback;
+
 	bool shared_fifo_flush;
 };
 
 struct jz4740_i2s {
 	struct regmap *regmap;
 
+	struct regmap_field *field_rx_fifo_thresh;
+	struct regmap_field *field_tx_fifo_thresh;
+	struct regmap_field *field_i2sdiv_capture;
+	struct regmap_field *field_i2sdiv_playback;
+
 	struct clk *clk_aic;
 	struct clk *clk_i2s;
 
@@ -238,12 +227,12 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+	struct regmap_field *div_field;
 	unsigned int sample_size;
-	uint32_t ctrl, div_reg;
+	uint32_t ctrl;
 	int div;
 
 	regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
-	regmap_read(i2s->regmap, JZ_REG_AIC_CLK_DIV, &div_reg);
 
 	div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
 
@@ -266,23 +255,16 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 		else
 			ctrl &= ~JZ_AIC_CTRL_MONO_TO_STEREO;
 
-		div_reg &= ~I2SDIV_DV_MASK;
-		div_reg |= (div - 1) << I2SDIV_DV_SHIFT;
+		div_field = i2s->field_i2sdiv_playback;
 	} else {
 		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK;
 		ctrl |= sample_size << JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET;
 
-		if (i2s->soc_info->version >= JZ_I2S_JZ4770) {
-			div_reg &= ~I2SDIV_IDV_MASK;
-			div_reg |= (div - 1) << I2SDIV_IDV_SHIFT;
-		} else {
-			div_reg &= ~I2SDIV_DV_MASK;
-			div_reg |= (div - 1) << I2SDIV_DV_SHIFT;
-		}
+		div_field = i2s->field_i2sdiv_capture;
 	}
 
 	regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);
-	regmap_write(i2s->regmap, JZ_REG_AIC_CLK_DIV, div_reg);
+	regmap_field_write(div_field, div - 1);
 
 	return 0;
 }
@@ -355,7 +337,6 @@ static int jz4740_i2s_resume(struct snd_soc_component *component)
 static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-	uint32_t conf;
 	int ret;
 
 	ret = clk_prepare_enable(i2s->clk_aic);
@@ -365,22 +346,14 @@ static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
 	snd_soc_dai_init_dma_data(dai, &i2s->playback_dma_data,
 		&i2s->capture_dma_data);
 
-	if (i2s->soc_info->version >= JZ_I2S_JZ4760) {
-		conf = (7 << JZ4760_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET) |
-			(8 << JZ4760_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET) |
-			JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
-			JZ_AIC_CONF_I2S |
-			JZ_AIC_CONF_INTERNAL_CODEC;
-	} else {
-		conf = (7 << JZ_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET) |
-			(8 << JZ_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET) |
-			JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
-			JZ_AIC_CONF_I2S |
-			JZ_AIC_CONF_INTERNAL_CODEC;
-	}
-
 	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
-	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, conf);
+
+	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
+		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
+		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
+
+	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
+	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
 
 	return 0;
 }
@@ -425,14 +398,20 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = {
 };
 
 static const struct i2s_soc_info jz4740_i2s_soc_info = {
-	.version = JZ_I2S_JZ4740,
-	.dai = &jz4740_i2s_dai,
-	.shared_fifo_flush = true,
+	.dai			= &jz4740_i2s_dai,
+	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 12, 15),
+	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
+	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.shared_fifo_flush	= true,
 };
 
 static const struct i2s_soc_info jz4760_i2s_soc_info = {
-	.version = JZ_I2S_JZ4760,
-	.dai = &jz4740_i2s_dai,
+	.dai			= &jz4740_i2s_dai,
+	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
+	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
+	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
 };
 
 static struct snd_soc_dai_driver jz4770_i2s_dai = {
@@ -454,13 +433,19 @@ static struct snd_soc_dai_driver jz4770_i2s_dai = {
 };
 
 static const struct i2s_soc_info jz4770_i2s_soc_info = {
-	.version = JZ_I2S_JZ4770,
-	.dai = &jz4770_i2s_dai,
+	.dai			= &jz4770_i2s_dai,
+	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
+	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
+	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
+	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
 };
 
 static const struct i2s_soc_info jz4780_i2s_soc_info = {
-	.version = JZ_I2S_JZ4780,
-	.dai = &jz4770_i2s_dai,
+	.dai			= &jz4770_i2s_dai,
+	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
+	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
+	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
+	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
 };
 
 static const struct snd_soc_component_driver jz4740_i2s_component = {
@@ -479,6 +464,36 @@ static const struct of_device_id jz4740_of_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, jz4740_of_matches);
 
+static int jz4740_i2s_init_regmap_fields(struct device *dev,
+					 struct jz4740_i2s *i2s)
+{
+	i2s->field_rx_fifo_thresh =
+		devm_regmap_field_alloc(dev, i2s->regmap,
+					i2s->soc_info->field_rx_fifo_thresh);
+	if (IS_ERR(i2s->field_rx_fifo_thresh))
+		return PTR_ERR(i2s->field_rx_fifo_thresh);
+
+	i2s->field_tx_fifo_thresh =
+		devm_regmap_field_alloc(dev, i2s->regmap,
+					i2s->soc_info->field_tx_fifo_thresh);
+	if (IS_ERR(i2s->field_tx_fifo_thresh))
+		return PTR_ERR(i2s->field_tx_fifo_thresh);
+
+	i2s->field_i2sdiv_capture =
+		devm_regmap_field_alloc(dev, i2s->regmap,
+					i2s->soc_info->field_i2sdiv_capture);
+	if (IS_ERR(i2s->field_i2sdiv_capture))
+		return PTR_ERR(i2s->field_i2sdiv_capture);
+
+	i2s->field_i2sdiv_playback =
+		devm_regmap_field_alloc(dev, i2s->regmap,
+					i2s->soc_info->field_i2sdiv_playback);
+	if (IS_ERR(i2s->field_i2sdiv_playback))
+		return PTR_ERR(i2s->field_i2sdiv_playback);
+
+	return 0;
+}
+
 static const struct regmap_config jz4740_i2s_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
@@ -523,6 +538,10 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev)
 	if (IS_ERR(i2s->regmap))
 		return PTR_ERR(i2s->regmap);
 
+	ret = jz4740_i2s_init_regmap_fields(dev, i2s);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, i2s);
 
 	ret = devm_snd_soc_register_component(dev, &jz4740_i2s_component,
-- 
2.35.1

[PATCH v4 05/11] ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback

[Aidan MacDonald]

Get rid of a couple of macros and improve readability by using
FIELD_PREP() and GENMASK() for the sample size setting.

Acked-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 043f100a9cfa..d0791dfa9c7b 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -3,6 +3,7 @@
  *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
  */
 
+#include <linux/bitfield.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -42,8 +43,8 @@
 #define JZ_AIC_CONF_SYNC_CLK_MASTER BIT(1)
 #define JZ_AIC_CONF_ENABLE BIT(0)
 
-#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK (0x7 << 19)
-#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK (0x7 << 16)
+#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE GENMASK(21, 19)
+#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE GENMASK(18, 16)
 #define JZ_AIC_CTRL_ENABLE_RX_DMA BIT(15)
 #define JZ_AIC_CTRL_ENABLE_TX_DMA BIT(14)
 #define JZ_AIC_CTRL_MONO_TO_STEREO BIT(11)
@@ -61,9 +62,6 @@
 #define JZ4760_AIC_CTRL_TFLUSH BIT(8)
 #define JZ4760_AIC_CTRL_RFLUSH BIT(7)
 
-#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
-#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
-
 #define JZ_AIC_I2S_FMT_DISABLE_BIT_CLK BIT(12)
 #define JZ_AIC_I2S_FMT_DISABLE_BIT_ICLK BIT(13)
 #define JZ_AIC_I2S_FMT_ENABLE_SYS_CLK BIT(4)
@@ -248,8 +246,9 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		ctrl &= ~JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK;
-		ctrl |= sample_size << JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET;
+		ctrl &= ~JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE;
+		ctrl |= FIELD_PREP(JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE, sample_size);
+
 		if (params_channels(params) == 1)
 			ctrl |= JZ_AIC_CTRL_MONO_TO_STEREO;
 		else
@@ -257,8 +256,8 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 
 		div_field = i2s->field_i2sdiv_playback;
 	} else {
-		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK;
-		ctrl |= sample_size << JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET;
+		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE;
+		ctrl |= FIELD_PREP(JZ_AIC_CTRL_INPUT_SAMPLE_SIZE, sample_size);
 
 		div_field = i2s->field_i2sdiv_capture;
 	}
-- 
2.35.1

[PATCH v4 06/11] ASoC: jz4740-i2s: Align macro values and sort includes

[Aidan MacDonald]

Some purely cosmetic changes: line up all the macro values to
make things easier to read and sort the includes alphabetically.

Acked-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 72 +++++++++++++++++------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index d0791dfa9c7b..0dcc658b3784 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -4,6 +4,9 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -13,11 +16,6 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-#include <linux/clk.h>
-#include <linux/delay.h>
-
-#include <linux/dma-mapping.h>
-
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -35,39 +33,39 @@
 #define JZ_REG_AIC_CLK_DIV	0x30
 #define JZ_REG_AIC_FIFO		0x34
 
-#define JZ_AIC_CONF_OVERFLOW_PLAY_LAST BIT(6)
-#define JZ_AIC_CONF_INTERNAL_CODEC BIT(5)
-#define JZ_AIC_CONF_I2S BIT(4)
-#define JZ_AIC_CONF_RESET BIT(3)
-#define JZ_AIC_CONF_BIT_CLK_MASTER BIT(2)
-#define JZ_AIC_CONF_SYNC_CLK_MASTER BIT(1)
-#define JZ_AIC_CONF_ENABLE BIT(0)
-
-#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE GENMASK(21, 19)
-#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE GENMASK(18, 16)
-#define JZ_AIC_CTRL_ENABLE_RX_DMA BIT(15)
-#define JZ_AIC_CTRL_ENABLE_TX_DMA BIT(14)
-#define JZ_AIC_CTRL_MONO_TO_STEREO BIT(11)
-#define JZ_AIC_CTRL_SWITCH_ENDIANNESS BIT(10)
-#define JZ_AIC_CTRL_SIGNED_TO_UNSIGNED BIT(9)
+#define JZ_AIC_CONF_OVERFLOW_PLAY_LAST	BIT(6)
+#define JZ_AIC_CONF_INTERNAL_CODEC	BIT(5)
+#define JZ_AIC_CONF_I2S			BIT(4)
+#define JZ_AIC_CONF_RESET		BIT(3)
+#define JZ_AIC_CONF_BIT_CLK_MASTER	BIT(2)
+#define JZ_AIC_CONF_SYNC_CLK_MASTER	BIT(1)
+#define JZ_AIC_CONF_ENABLE		BIT(0)
+
+#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE	GENMASK(21, 19)
+#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE	GENMASK(18, 16)
+#define JZ_AIC_CTRL_ENABLE_RX_DMA	BIT(15)
+#define JZ_AIC_CTRL_ENABLE_TX_DMA	BIT(14)
+#define JZ_AIC_CTRL_MONO_TO_STEREO	BIT(11)
+#define JZ_AIC_CTRL_SWITCH_ENDIANNESS	BIT(10)
+#define JZ_AIC_CTRL_SIGNED_TO_UNSIGNED	BIT(9)
 #define JZ_AIC_CTRL_FLUSH		BIT(8)
-#define JZ_AIC_CTRL_ENABLE_ROR_INT BIT(6)
-#define JZ_AIC_CTRL_ENABLE_TUR_INT BIT(5)
-#define JZ_AIC_CTRL_ENABLE_RFS_INT BIT(4)
-#define JZ_AIC_CTRL_ENABLE_TFS_INT BIT(3)
-#define JZ_AIC_CTRL_ENABLE_LOOPBACK BIT(2)
-#define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1)
-#define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0)
-
-#define JZ4760_AIC_CTRL_TFLUSH BIT(8)
-#define JZ4760_AIC_CTRL_RFLUSH BIT(7)
-
-#define JZ_AIC_I2S_FMT_DISABLE_BIT_CLK BIT(12)
-#define JZ_AIC_I2S_FMT_DISABLE_BIT_ICLK BIT(13)
-#define JZ_AIC_I2S_FMT_ENABLE_SYS_CLK BIT(4)
-#define JZ_AIC_I2S_FMT_MSB BIT(0)
-
-#define JZ_AIC_I2S_STATUS_BUSY BIT(2)
+#define JZ_AIC_CTRL_ENABLE_ROR_INT	BIT(6)
+#define JZ_AIC_CTRL_ENABLE_TUR_INT	BIT(5)
+#define JZ_AIC_CTRL_ENABLE_RFS_INT	BIT(4)
+#define JZ_AIC_CTRL_ENABLE_TFS_INT	BIT(3)
+#define JZ_AIC_CTRL_ENABLE_LOOPBACK	BIT(2)
+#define JZ_AIC_CTRL_ENABLE_PLAYBACK	BIT(1)
+#define JZ_AIC_CTRL_ENABLE_CAPTURE	BIT(0)
+
+#define JZ4760_AIC_CTRL_TFLUSH		BIT(8)
+#define JZ4760_AIC_CTRL_RFLUSH		BIT(7)
+
+#define JZ_AIC_I2S_FMT_DISABLE_BIT_CLK	BIT(12)
+#define JZ_AIC_I2S_FMT_DISABLE_BIT_ICLK	BIT(13)
+#define JZ_AIC_I2S_FMT_ENABLE_SYS_CLK	BIT(4)
+#define JZ_AIC_I2S_FMT_MSB		BIT(0)
+
+#define JZ_AIC_I2S_STATUS_BUSY		BIT(2)
 
 struct i2s_soc_info {
 	struct snd_soc_dai_driver *dai;
-- 
2.35.1

[PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Aidan MacDonald]

On some Ingenic SoCs, such as the X1000, there is a programmable
divider used to generate the I2S system clock from a PLL, rather
than a fixed PLL/2 clock. It doesn't make much sense to call the
clock "pll half" on those SoCs, so the clock name should really be
a SoC-dependent value.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 0dcc658b3784..a41398c24d0e 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -75,6 +75,8 @@ struct i2s_soc_info {
 	struct reg_field field_i2sdiv_capture;
 	struct reg_field field_i2sdiv_playback;
 
+	const char *pll_clk_name;
+
 	bool shared_fifo_flush;
 };
 
@@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 		clk_set_parent(i2s->clk_i2s, parent);
 		break;
 	case JZ4740_I2S_CLKSRC_PLL:
-		parent = clk_get(NULL, "pll half");
+		parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
 		if (IS_ERR(parent))
 			return PTR_ERR(parent);
 		clk_set_parent(i2s->clk_i2s, parent);
@@ -400,6 +402,7 @@ static const struct i2s_soc_info jz4740_i2s_soc_info = {
 	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
 	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
 	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.pll_clk_name		= "pll half",
 	.shared_fifo_flush	= true,
 };
 
@@ -409,6 +412,7 @@ static const struct i2s_soc_info jz4760_i2s_soc_info = {
 	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
 	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
 	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.pll_clk_name		= "pll half",
 };
 
 static struct snd_soc_dai_driver jz4770_i2s_dai = {
@@ -435,6 +439,7 @@ static const struct i2s_soc_info jz4770_i2s_soc_info = {
 	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
 	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
 	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.pll_clk_name		= "pll half",
 };
 
 static const struct i2s_soc_info jz4780_i2s_soc_info = {
@@ -443,6 +448,7 @@ static const struct i2s_soc_info jz4780_i2s_soc_info = {
 	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
 	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
 	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
+	.pll_clk_name		= "pll half",
 };
 
 static const struct snd_soc_component_driver jz4740_i2s_component = {
-- 
2.35.1

[PATCH v4 08/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats

[Aidan MacDonald]

The audio controller on JZ47xx SoCs can transfer 20- and 24-bit
samples in the FIFO, so allow those formats to be used with the
I2S driver. Although the FIFO doesn't care about the in-memory
sample format, we only support 4-byte format variants because the
DMA controller on these SoCs cannot transfer in 3-byte multiples.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index a41398c24d0e..9be2f3f1b376 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -238,9 +238,15 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S8:
 		sample_size = 0;
 		break;
-	case SNDRV_PCM_FORMAT_S16:
+	case SNDRV_PCM_FORMAT_S16_LE:
 		sample_size = 1;
 		break;
+	case SNDRV_PCM_FORMAT_S20_LE:
+		sample_size = 3;
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		sample_size = 4;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -375,7 +381,9 @@ static const struct snd_soc_dai_ops jz4740_i2s_dai_ops = {
 };
 
 #define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
-		SNDRV_PCM_FMTBIT_S16_LE)
+			 SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S20_LE | \
+			 SNDRV_PCM_FMTBIT_S24_LE)
 
 static struct snd_soc_dai_driver jz4740_i2s_dai = {
 	.probe = jz4740_i2s_dai_probe,
-- 
2.35.1

[PATCH v4 09/11] ASoC: jz4740-i2s: Support continuous sample rate

[Aidan MacDonald]

The I2S controller on JZ47xx SoCs doesn't impose restrictions on
sample rate and the driver doesn't make any assumptions about it,
so the DAI should advertise a continuous sample rate range.

Acked-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 9be2f3f1b376..70b9d28a40ce 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -391,13 +391,13 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = {
 	.playback = {
 		.channels_min = 1,
 		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_48000,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
 		.formats = JZ4740_I2S_FMTS,
 	},
 	.capture = {
 		.channels_min = 2,
 		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_48000,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
 		.formats = JZ4740_I2S_FMTS,
 	},
 	.symmetric_rate = 1,
@@ -429,13 +429,13 @@ static struct snd_soc_dai_driver jz4770_i2s_dai = {
 	.playback = {
 		.channels_min = 1,
 		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_48000,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
 		.formats = JZ4740_I2S_FMTS,
 	},
 	.capture = {
 		.channels_min = 2,
 		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_48000,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
 		.formats = JZ4740_I2S_FMTS,
 	},
 	.ops = &jz4740_i2s_dai_ops,
-- 
2.35.1

[PATCH v4 10/11] ASoC: jz4740-i2s: Move component functions near the component driver

[Aidan MacDonald]

Move the component suspend/resume functions near the definition
of the component driver to emphasize that they're unrelated to
the DAI functions.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 72 +++++++++++++++++------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 70b9d28a40ce..5db73f12efcf 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -303,42 +303,6 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 	return ret;
 }
 
-static int jz4740_i2s_suspend(struct snd_soc_component *component)
-{
-	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
-
-	if (snd_soc_component_active(component)) {
-		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
-		clk_disable_unprepare(i2s->clk_i2s);
-	}
-
-	clk_disable_unprepare(i2s->clk_aic);
-
-	return 0;
-}
-
-static int jz4740_i2s_resume(struct snd_soc_component *component)
-{
-	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
-	int ret;
-
-	ret = clk_prepare_enable(i2s->clk_aic);
-	if (ret)
-		return ret;
-
-	if (snd_soc_component_active(component)) {
-		ret = clk_prepare_enable(i2s->clk_i2s);
-		if (ret) {
-			clk_disable_unprepare(i2s->clk_aic);
-			return ret;
-		}
-
-		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
-	}
-
-	return 0;
-}
-
 static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -459,6 +423,42 @@ static const struct i2s_soc_info jz4780_i2s_soc_info = {
 	.pll_clk_name		= "pll half",
 };
 
+static int jz4740_i2s_suspend(struct snd_soc_component *component)
+{
+	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+
+	if (snd_soc_component_active(component)) {
+		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
+		clk_disable_unprepare(i2s->clk_i2s);
+	}
+
+	clk_disable_unprepare(i2s->clk_aic);
+
+	return 0;
+}
+
+static int jz4740_i2s_resume(struct snd_soc_component *component)
+{
+	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ret = clk_prepare_enable(i2s->clk_aic);
+	if (ret)
+		return ret;
+
+	if (snd_soc_component_active(component)) {
+		ret = clk_prepare_enable(i2s->clk_i2s);
+		if (ret) {
+			clk_disable_unprepare(i2s->clk_aic);
+			return ret;
+		}
+
+		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver jz4740_i2s_component = {
 	.name			= "jz4740-i2s",
 	.suspend		= jz4740_i2s_suspend,
-- 
2.35.1

[PATCH v4 11/11] ASoC: jz4740-i2s: Refactor DAI probe/remove ops as component ops

[Aidan MacDonald]

Move most of the DAI probe/remove logic into component ops.
This makes things more consistent because the AIC clock is
now managed solely from the component side. And it makes it
easier to add codec switching support later on.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 54 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 5db73f12efcf..d99a19bc5166 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -306,32 +306,10 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
 {
 	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-	int ret;
-
-	ret = clk_prepare_enable(i2s->clk_aic);
-	if (ret)
-		return ret;
 
 	snd_soc_dai_init_dma_data(dai, &i2s->playback_dma_data,
 		&i2s->capture_dma_data);
 
-	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
-
-	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
-		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
-		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
-
-	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
-	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
-
-	return 0;
-}
-
-static int jz4740_i2s_dai_remove(struct snd_soc_dai *dai)
-{
-	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
-	clk_disable_unprepare(i2s->clk_aic);
 	return 0;
 }
 
@@ -351,7 +329,6 @@ static const struct snd_soc_dai_ops jz4740_i2s_dai_ops = {
 
 static struct snd_soc_dai_driver jz4740_i2s_dai = {
 	.probe = jz4740_i2s_dai_probe,
-	.remove = jz4740_i2s_dai_remove,
 	.playback = {
 		.channels_min = 1,
 		.channels_max = 2,
@@ -389,7 +366,6 @@ static const struct i2s_soc_info jz4760_i2s_soc_info = {
 
 static struct snd_soc_dai_driver jz4770_i2s_dai = {
 	.probe = jz4740_i2s_dai_probe,
-	.remove = jz4740_i2s_dai_remove,
 	.playback = {
 		.channels_min = 1,
 		.channels_max = 2,
@@ -459,8 +435,38 @@ static int jz4740_i2s_resume(struct snd_soc_component *component)
 	return 0;
 }
 
+static int jz4740_i2s_probe(struct snd_soc_component *component)
+{
+	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ret = clk_prepare_enable(i2s->clk_aic);
+	if (ret)
+		return ret;
+
+	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
+
+	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
+		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
+		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
+
+	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
+	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
+
+	return 0;
+}
+
+static void jz4740_i2s_remove(struct snd_soc_component *component)
+{
+	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+
+	clk_disable_unprepare(i2s->clk_aic);
+}
+
 static const struct snd_soc_component_driver jz4740_i2s_component = {
 	.name			= "jz4740-i2s",
+	.probe			= jz4740_i2s_probe,
+	.remove			= jz4740_i2s_remove,
 	.suspend		= jz4740_i2s_suspend,
 	.resume			= jz4740_i2s_resume,
 	.legacy_dai_naming	= 1,
-- 
2.35.1

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Zhou Yanjie]

Hi Aidan,

On 2022/7/9 上午12:02, Aidan MacDonald wrote:
> On some Ingenic SoCs, such as the X1000, there is a programmable
> divider used to generate the I2S system clock from a PLL, rather
> than a fixed PLL/2 clock. It doesn't make much sense to call the
> clock "pll half" on those SoCs, so the clock name should really be
> a SoC-dependent value.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>   sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
> index 0dcc658b3784..a41398c24d0e 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -75,6 +75,8 @@ struct i2s_soc_info {
>   	struct reg_field field_i2sdiv_capture;
>   	struct reg_field field_i2sdiv_playback;
>   
> +	const char *pll_clk_name;
> +
>   	bool shared_fifo_flush;
>   };
>   
> @@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
>   		clk_set_parent(i2s->clk_i2s, parent);
>   		break;
>   	case JZ4740_I2S_CLKSRC_PLL:
> -		parent = clk_get(NULL, "pll half");
> +		parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
>   		if (IS_ERR(parent))
>   			return PTR_ERR(parent);
>   		clk_set_parent(i2s->clk_i2s, parent);
> @@ -400,6 +402,7 @@ static const struct i2s_soc_info jz4740_i2s_soc_info = {
>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>   	.shared_fifo_flush	= true,
>   };
>   
> @@ -409,6 +412,7 @@ static const struct i2s_soc_info jz4760_i2s_soc_info = {
>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>   };


Since JZ4760, according to the description of the I2SCDR register,
Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
so it seems also inappropriate to use "pll half" for these SoCs.


>   
>   static struct snd_soc_dai_driver jz4770_i2s_dai = {
> @@ -435,6 +439,7 @@ static const struct i2s_soc_info jz4770_i2s_soc_info = {
>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>   };


Same here.


>   
>   static const struct i2s_soc_info jz4780_i2s_soc_info = {
> @@ -443,6 +448,7 @@ static const struct i2s_soc_info jz4780_i2s_soc_info = {
>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>   };
>   


Same here.


Thanks and best regards!


>   static const struct snd_soc_component_driver jz4740_i2s_component = {

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

Hi Zhou,

Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie 
<zhouyu@wanyeetech.com> a écrit :
> Hi Aidan,
> 
> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>> On some Ingenic SoCs, such as the X1000, there is a programmable
>> divider used to generate the I2S system clock from a PLL, rather
>> than a fixed PLL/2 clock. It doesn't make much sense to call the
>> clock "pll half" on those SoCs, so the clock name should really be
>> a SoC-dependent value.
>> 
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>   sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
>> b/sound/soc/jz4740/jz4740-i2s.c
>> index 0dcc658b3784..a41398c24d0e 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -75,6 +75,8 @@ struct i2s_soc_info {
>>   	struct reg_field field_i2sdiv_capture;
>>   	struct reg_field field_i2sdiv_playback;
>>   \x7f+	const char *pll_clk_name;
>> +
>>   	bool shared_fifo_flush;
>>   };
>>   \x7f@@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct 
>> snd_soc_dai *dai, int clk_id,
>>   		clk_set_parent(i2s->clk_i2s, parent);
>>   		break;
>>   	case JZ4740_I2S_CLKSRC_PLL:
>> -		parent = clk_get(NULL, "pll half");
>> +		parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
>>   		if (IS_ERR(parent))
>>   			return PTR_ERR(parent);
>>   		clk_set_parent(i2s->clk_i2s, parent);
>> @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
>> jz4740_i2s_soc_info = {
>>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>> +	.pll_clk_name		= "pll half",
>>   	.shared_fifo_flush	= true,
>>   };
>>   \x7f@@ -409,6 +412,7 @@ static const struct i2s_soc_info 
>> jz4760_i2s_soc_info = {
>>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>> +	.pll_clk_name		= "pll half",
>>   };
> 
> 
> Since JZ4760, according to the description of the I2SCDR register,
> Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
> so it seems also inappropriate to use "pll half" for these SoCs.

The device tree passes the clock as "pll half". So the driver should 
use this name as well...

Cheers,
-Paul

>>   \x7f  static struct snd_soc_dai_driver jz4770_i2s_dai = {
>> @@ -435,6 +439,7 @@ static const struct i2s_soc_info 
>> jz4770_i2s_soc_info = {
>>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>> +	.pll_clk_name		= "pll half",
>>   };
> 
> 
> Same here.
> 
> 
>>   \x7f  static const struct i2s_soc_info jz4780_i2s_soc_info = {
>> @@ -443,6 +448,7 @@ static const struct i2s_soc_info 
>> jz4780_i2s_soc_info = {
>>   	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>   	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>   	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>> +	.pll_clk_name		= "pll half",
>>   };
>> 
> 
> Same here.
> 
> 
> Thanks and best regards!
> 
> 
>>   static const struct snd_soc_component_driver jz4740_i2s_component 
>> = {

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Zhou Yanjie]

Hi Paul,

On 2022/7/13 下午11:07, Paul Cercueil wrote:
> Hi Zhou,
>
> Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie 
> <zhouyu@wanyeetech.com> a écrit :
>> Hi Aidan,
>>
>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>> On some Ingenic SoCs, such as the X1000, there is a programmable
>>> divider used to generate the I2S system clock from a PLL, rather
>>> than a fixed PLL/2 clock. It doesn't make much sense to call the
>>> clock "pll half" on those SoCs, so the clock name should really be
>>> a SoC-dependent value.
>>>
>>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>>> ---
>>>   sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
>>> b/sound/soc/jz4740/jz4740-i2s.c
>>> index 0dcc658b3784..a41398c24d0e 100644
>>> --- a/sound/soc/jz4740/jz4740-i2s.c
>>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>>> @@ -75,6 +75,8 @@ struct i2s_soc_info {
>>>       struct reg_field field_i2sdiv_capture;
>>>       struct reg_field field_i2sdiv_playback;
>>>   \x7f+    const char *pll_clk_name;
>>> +
>>>       bool shared_fifo_flush;
>>>   };
>>>   \x7f@@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct 
>>> snd_soc_dai *dai, int clk_id,
>>>           clk_set_parent(i2s->clk_i2s, parent);
>>>           break;
>>>       case JZ4740_I2S_CLKSRC_PLL:
>>> -        parent = clk_get(NULL, "pll half");
>>> +        parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
>>>           if (IS_ERR(parent))
>>>               return PTR_ERR(parent);
>>>           clk_set_parent(i2s->clk_i2s, parent);
>>> @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
>>> jz4740_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>       .shared_fifo_flush    = true,
>>>   };
>>>   \x7f@@ -409,6 +412,7 @@ static const struct i2s_soc_info 
>>> jz4760_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>
>>
>> Since JZ4760, according to the description of the I2SCDR register,
>> Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
>> so it seems also inappropriate to use "pll half" for these SoCs.
>
> The device tree passes the clock as "pll half". So the driver should 
> use this name as well...


I see...

It seems that the device tree of JZ4770 has used "pll half" already,
but there is no "pll half" used anywhere in the device tree of JZ4780,
maybe we can keep the pll_clk_name of JZ4770 as "pll half", and change
the pll_clk_name of JZ4780 to a more reasonable name.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>>>   \x7f  static struct snd_soc_dai_driver jz4770_i2s_dai = {
>>> @@ -435,6 +439,7 @@ static const struct i2s_soc_info 
>>> jz4770_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>
>>
>> Same here.
>>
>>
>>>   \x7f  static const struct i2s_soc_info jz4780_i2s_soc_info = {
>>> @@ -443,6 +448,7 @@ static const struct i2s_soc_info 
>>> jz4780_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>>
>>
>> Same here.
>>
>>
>> Thanks and best regards!
>>
>>
>>>   static const struct snd_soc_component_driver jz4740_i2s_component = {
>

Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:34 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> On the JZ4740, there is a single bit that flushes (empties) both
> the transmit and receive FIFO. Later SoCs have independent flush
> bits for each FIFO, which allows us to flush the right FIFO when
> starting up a stream.
> 
> This also fixes a bug: since we were only setting the JZ4740's
> flush bit, which corresponds to the TX FIFO flush bit on other
> SoCs, other SoCs were not having their RX FIFO flushed at all.
> 
> Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  sound/soc/jz4740/jz4740-i2s.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index ecd8df70d39c..576f31f9d734 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -64,6 +64,9 @@
>  #define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1)
>  #define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0)
> 
> +#define JZ4760_AIC_CTRL_TFLUSH BIT(8)
> +#define JZ4760_AIC_CTRL_RFLUSH BIT(7)

Just rename JZ_AIC_CTRL_FLUSH to JZ_AIC_CTRL_TFLUSH and introduce 
JZ_AIC_CTRL_RLUSH.

> +
>  #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
>  #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
> 
> @@ -90,6 +93,8 @@ enum jz47xx_i2s_version {
>  struct i2s_soc_info {
>  	enum jz47xx_i2s_version version;
>  	struct snd_soc_dai_driver *dai;
> +
> +	bool shared_fifo_flush;
>  };
> 
>  struct jz4740_i2s {
> @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct 
> snd_pcm_substream *substream,
>  	uint32_t conf, ctrl;
>  	int ret;
> 
> +	/*
> +	 * When we can flush FIFOs independently, only flush the
> +	 * FIFO that is starting up.
> +	 */
> +	if (!i2s->soc_info->shared_fifo_flush) {
> +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
> +		else
> +			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
> +
> +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +	}

Wouldn't it be simpler to do one single if/else? And hy is one checked 
before the (snd_soc_dai_active(dai)) check, and the other is checked 
after?

You could do something like this:

ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);

if (i2s->soc_info->shared_fifo_flush ||
    substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
    ctrl |= JZ_AIC_CTRL_TFLUSH;
} else {
    ctrl |= JZ_AIC_CTRL_RFLUSH;
}

jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);

Cheers,
-Paul

> +
>  	if (snd_soc_dai_active(dai))
>  		return 0;
> 
> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -	ctrl |= JZ_AIC_CTRL_FLUSH;
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +	/*
> +	 * When there is a shared flush bit for both FIFOs we can
> +	 * only flush the FIFOs if no other stream has started.
> +	 */
> +	if (i2s->soc_info->shared_fifo_flush) {
> +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> +		ctrl |= JZ_AIC_CTRL_FLUSH;
> +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +	}
> 
>  	ret = clk_prepare_enable(i2s->clk_i2s);
>  	if (ret)
> @@ -444,6 +470,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = 
> {
>  static const struct i2s_soc_info jz4740_i2s_soc_info = {
>  	.version = JZ_I2S_JZ4740,
>  	.dai = &jz4740_i2s_dai,
> +	.shared_fifo_flush = true,
>  };
> 
>  static const struct i2s_soc_info jz4760_i2s_soc_info = {
> --
> 2.35.1
> 

Re: [PATCH v4 02/11] ASoC: jz4740-i2s: Remove unused 'mem' resource

[Paul Cercueil]

Hi,

Le ven., juil. 8 2022 at 17:02:35 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> This isn't used and doesn't need to be in the private data struct.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 576f31f9d734..adf896333584 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -98,7 +98,6 @@ struct i2s_soc_info {
>  };
> 
>  struct jz4740_i2s {
> -	struct resource *mem;
>  	void __iomem *base;
> 
>  	struct clk *clk_aic;
> --
> 2.35.1
> 

Re: [PATCH v4 04/11] ASoC: jz4740-i2s: Simplify using regmap fields

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:37 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> The differences between register fields on different SoC versions
> can be abstracted away using the regmap field API. This is easier
> to understand and extend than comparisons based on the version ID.
> Since the version IDs are unused after this change, remove them at
> the same time, and remove unused macros.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 135 
> +++++++++++++++++++---------------
>  1 file changed, 77 insertions(+), 58 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 1393b383a886..043f100a9cfa 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -34,8 +34,6 @@
>  #define JZ_REG_AIC_CLK_DIV	0x30
>  #define JZ_REG_AIC_FIFO		0x34
> 
> -#define JZ_AIC_CONF_FIFO_RX_THRESHOLD_MASK (0xf << 12)
> -#define JZ_AIC_CONF_FIFO_TX_THRESHOLD_MASK (0xf <<  8)
>  #define JZ_AIC_CONF_OVERFLOW_PLAY_LAST BIT(6)
>  #define JZ_AIC_CONF_INTERNAL_CODEC BIT(5)
>  #define JZ_AIC_CONF_I2S BIT(4)
> @@ -44,11 +42,6 @@
>  #define JZ_AIC_CONF_SYNC_CLK_MASTER BIT(1)
>  #define JZ_AIC_CONF_ENABLE BIT(0)
> 
> -#define JZ_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET 12
> -#define JZ_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET 8
> -#define JZ4760_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET 24
> -#define JZ4760_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET 16
> -
>  #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK (0x7 << 19)
>  #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK (0x7 << 16)
>  #define JZ_AIC_CTRL_ENABLE_RX_DMA BIT(15)
> @@ -78,29 +71,25 @@
> 
>  #define JZ_AIC_I2S_STATUS_BUSY BIT(2)
> 
> -#define JZ_AIC_CLK_DIV_MASK 0xf
> -#define I2SDIV_DV_SHIFT 0
> -#define I2SDIV_DV_MASK (0xf << I2SDIV_DV_SHIFT)
> -#define I2SDIV_IDV_SHIFT 8
> -#define I2SDIV_IDV_MASK (0xf << I2SDIV_IDV_SHIFT)
> -
> -enum jz47xx_i2s_version {
> -	JZ_I2S_JZ4740,
> -	JZ_I2S_JZ4760,
> -	JZ_I2S_JZ4770,
> -	JZ_I2S_JZ4780,
> -};
> -
>  struct i2s_soc_info {
> -	enum jz47xx_i2s_version version;
>  	struct snd_soc_dai_driver *dai;
> 
> +	struct reg_field field_rx_fifo_thresh;
> +	struct reg_field field_tx_fifo_thresh;
> +	struct reg_field field_i2sdiv_capture;
> +	struct reg_field field_i2sdiv_playback;
> +
>  	bool shared_fifo_flush;
>  };
> 
>  struct jz4740_i2s {
>  	struct regmap *regmap;
> 
> +	struct regmap_field *field_rx_fifo_thresh;
> +	struct regmap_field *field_tx_fifo_thresh;
> +	struct regmap_field *field_i2sdiv_capture;
> +	struct regmap_field *field_i2sdiv_playback;
> +
>  	struct clk *clk_aic;
>  	struct clk *clk_i2s;
> 
> @@ -238,12 +227,12 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  	struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +	struct regmap_field *div_field;
>  	unsigned int sample_size;
> -	uint32_t ctrl, div_reg;
> +	uint32_t ctrl;
>  	int div;
> 
>  	regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
> -	regmap_read(i2s->regmap, JZ_REG_AIC_CLK_DIV, &div_reg);
> 
>  	div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
> 
> @@ -266,23 +255,16 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  		else
>  			ctrl &= ~JZ_AIC_CTRL_MONO_TO_STEREO;
> 
> -		div_reg &= ~I2SDIV_DV_MASK;
> -		div_reg |= (div - 1) << I2SDIV_DV_SHIFT;
> +		div_field = i2s->field_i2sdiv_playback;
>  	} else {
>  		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK;
>  		ctrl |= sample_size << JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET;
> 
> -		if (i2s->soc_info->version >= JZ_I2S_JZ4770) {
> -			div_reg &= ~I2SDIV_IDV_MASK;
> -			div_reg |= (div - 1) << I2SDIV_IDV_SHIFT;
> -		} else {
> -			div_reg &= ~I2SDIV_DV_MASK;
> -			div_reg |= (div - 1) << I2SDIV_DV_SHIFT;
> -		}
> +		div_field = i2s->field_i2sdiv_capture;
>  	}
> 
>  	regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);
> -	regmap_write(i2s->regmap, JZ_REG_AIC_CLK_DIV, div_reg);
> +	regmap_field_write(div_field, div - 1);
> 
>  	return 0;
>  }
> @@ -355,7 +337,6 @@ static int jz4740_i2s_resume(struct 
> snd_soc_component *component)
>  static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -	uint32_t conf;
>  	int ret;
> 
>  	ret = clk_prepare_enable(i2s->clk_aic);
> @@ -365,22 +346,14 @@ static int jz4740_i2s_dai_probe(struct 
> snd_soc_dai *dai)
>  	snd_soc_dai_init_dma_data(dai, &i2s->playback_dma_data,
>  		&i2s->capture_dma_data);
> 
> -	if (i2s->soc_info->version >= JZ_I2S_JZ4760) {
> -		conf = (7 << JZ4760_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET) |
> -			(8 << JZ4760_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET) |
> -			JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
> -			JZ_AIC_CONF_I2S |
> -			JZ_AIC_CONF_INTERNAL_CODEC;
> -	} else {
> -		conf = (7 << JZ_AIC_CONF_FIFO_RX_THRESHOLD_OFFSET) |
> -			(8 << JZ_AIC_CONF_FIFO_TX_THRESHOLD_OFFSET) |
> -			JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
> -			JZ_AIC_CONF_I2S |
> -			JZ_AIC_CONF_INTERNAL_CODEC;
> -	}
> -
>  	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
> -	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, conf);
> +
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
> +		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
> +		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
> +
> +	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
> +	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
> 
>  	return 0;
>  }
> @@ -425,14 +398,20 @@ static struct snd_soc_dai_driver jz4740_i2s_dai 
> = {
>  };
> 
>  static const struct i2s_soc_info jz4740_i2s_soc_info = {
> -	.version = JZ_I2S_JZ4740,
> -	.dai = &jz4740_i2s_dai,
> -	.shared_fifo_flush = true,
> +	.dai			= &jz4740_i2s_dai,
> +	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 12, 15),
> +	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
> +	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.shared_fifo_flush	= true,
>  };
> 
>  static const struct i2s_soc_info jz4760_i2s_soc_info = {
> -	.version = JZ_I2S_JZ4760,
> -	.dai = &jz4740_i2s_dai,
> +	.dai			= &jz4740_i2s_dai,
> +	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
> +	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
> +	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>  };
> 
>  static struct snd_soc_dai_driver jz4770_i2s_dai = {
> @@ -454,13 +433,19 @@ static struct snd_soc_dai_driver jz4770_i2s_dai 
> = {
>  };
> 
>  static const struct i2s_soc_info jz4770_i2s_soc_info = {
> -	.version = JZ_I2S_JZ4770,
> -	.dai = &jz4770_i2s_dai,
> +	.dai			= &jz4770_i2s_dai,
> +	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
> +	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
> +	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
> +	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>  };
> 
>  static const struct i2s_soc_info jz4780_i2s_soc_info = {
> -	.version = JZ_I2S_JZ4780,
> -	.dai = &jz4770_i2s_dai,
> +	.dai			= &jz4770_i2s_dai,
> +	.field_rx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 24, 27),
> +	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
> +	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
> +	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>  };
> 
>  static const struct snd_soc_component_driver jz4740_i2s_component = {
> @@ -479,6 +464,36 @@ static const struct of_device_id 
> jz4740_of_matches[] = {
>  };
>  MODULE_DEVICE_TABLE(of, jz4740_of_matches);
> 
> +static int jz4740_i2s_init_regmap_fields(struct device *dev,
> +					 struct jz4740_i2s *i2s)
> +{
> +	i2s->field_rx_fifo_thresh =
> +		devm_regmap_field_alloc(dev, i2s->regmap,
> +					i2s->soc_info->field_rx_fifo_thresh);
> +	if (IS_ERR(i2s->field_rx_fifo_thresh))
> +		return PTR_ERR(i2s->field_rx_fifo_thresh);
> +
> +	i2s->field_tx_fifo_thresh =
> +		devm_regmap_field_alloc(dev, i2s->regmap,
> +					i2s->soc_info->field_tx_fifo_thresh);
> +	if (IS_ERR(i2s->field_tx_fifo_thresh))
> +		return PTR_ERR(i2s->field_tx_fifo_thresh);
> +
> +	i2s->field_i2sdiv_capture =
> +		devm_regmap_field_alloc(dev, i2s->regmap,
> +					i2s->soc_info->field_i2sdiv_capture);
> +	if (IS_ERR(i2s->field_i2sdiv_capture))
> +		return PTR_ERR(i2s->field_i2sdiv_capture);
> +
> +	i2s->field_i2sdiv_playback =
> +		devm_regmap_field_alloc(dev, i2s->regmap,
> +					i2s->soc_info->field_i2sdiv_playback);
> +	if (IS_ERR(i2s->field_i2sdiv_playback))
> +		return PTR_ERR(i2s->field_i2sdiv_playback);
> +
> +	return 0;
> +}
> +
>  static const struct regmap_config jz4740_i2s_regmap_config = {
>  	.reg_bits	= 32,
>  	.reg_stride	= 4,
> @@ -523,6 +538,10 @@ static int jz4740_i2s_dev_probe(struct 
> platform_device *pdev)
>  	if (IS_ERR(i2s->regmap))
>  		return PTR_ERR(i2s->regmap);
> 
> +	ret = jz4740_i2s_init_regmap_fields(dev, i2s);
> +	if (ret)
> +		return ret;
> +
>  	platform_set_drvdata(pdev, i2s);
> 
>  	ret = devm_snd_soc_register_component(dev, &jz4740_i2s_component,
> --
> 2.35.1
> 

Re: [PATCH v4 05/11] ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:38 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Get rid of a couple of macros and improve readability by using
> FIELD_PREP() and GENMASK() for the sample size setting.
> 
> Acked-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 043f100a9cfa..d0791dfa9c7b 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -3,6 +3,7 @@
>   *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
>   */
> 
> +#include <linux/bitfield.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> @@ -42,8 +43,8 @@
>  #define JZ_AIC_CONF_SYNC_CLK_MASTER BIT(1)
>  #define JZ_AIC_CONF_ENABLE BIT(0)
> 
> -#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK (0x7 << 19)
> -#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK (0x7 << 16)
> +#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE GENMASK(21, 19)
> +#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE GENMASK(18, 16)
>  #define JZ_AIC_CTRL_ENABLE_RX_DMA BIT(15)
>  #define JZ_AIC_CTRL_ENABLE_TX_DMA BIT(14)
>  #define JZ_AIC_CTRL_MONO_TO_STEREO BIT(11)
> @@ -61,9 +62,6 @@
>  #define JZ4760_AIC_CTRL_TFLUSH BIT(8)
>  #define JZ4760_AIC_CTRL_RFLUSH BIT(7)
> 
> -#define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
> -#define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
> -
>  #define JZ_AIC_I2S_FMT_DISABLE_BIT_CLK BIT(12)
>  #define JZ_AIC_I2S_FMT_DISABLE_BIT_ICLK BIT(13)
>  #define JZ_AIC_I2S_FMT_ENABLE_SYS_CLK BIT(4)
> @@ -248,8 +246,9 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  	}
> 
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -		ctrl &= ~JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_MASK;
> -		ctrl |= sample_size << JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET;
> +		ctrl &= ~JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE;
> +		ctrl |= FIELD_PREP(JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE, sample_size);
> +
>  		if (params_channels(params) == 1)
>  			ctrl |= JZ_AIC_CTRL_MONO_TO_STEREO;
>  		else
> @@ -257,8 +256,8 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
> 
>  		div_field = i2s->field_i2sdiv_playback;
>  	} else {
> -		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_MASK;
> -		ctrl |= sample_size << JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET;
> +		ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE;
> +		ctrl |= FIELD_PREP(JZ_AIC_CTRL_INPUT_SAMPLE_SIZE, sample_size);
> 
>  		div_field = i2s->field_i2sdiv_capture;
>  	}
> --
> 2.35.1
> 

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:40 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> On some Ingenic SoCs, such as the X1000, there is a programmable
> divider used to generate the I2S system clock from a PLL, rather
> than a fixed PLL/2 clock. It doesn't make much sense to call the
> clock "pll half" on those SoCs, so the clock name should really be
> a SoC-dependent value.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 0dcc658b3784..a41398c24d0e 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -75,6 +75,8 @@ struct i2s_soc_info {
>  	struct reg_field field_i2sdiv_capture;
>  	struct reg_field field_i2sdiv_playback;
> 
> +	const char *pll_clk_name;
> +
>  	bool shared_fifo_flush;
>  };
> 
> @@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct 
> snd_soc_dai *dai, int clk_id,
>  		clk_set_parent(i2s->clk_i2s, parent);
>  		break;
>  	case JZ4740_I2S_CLKSRC_PLL:
> -		parent = clk_get(NULL, "pll half");
> +		parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
>  		if (IS_ERR(parent))
>  			return PTR_ERR(parent);
>  		clk_set_parent(i2s->clk_i2s, parent);
> @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
> jz4740_i2s_soc_info = {
>  	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>  	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>  	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>  	.shared_fifo_flush	= true,
>  };
> 
> @@ -409,6 +412,7 @@ static const struct i2s_soc_info 
> jz4760_i2s_soc_info = {
>  	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>  	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>  	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>  };
> 
>  static struct snd_soc_dai_driver jz4770_i2s_dai = {
> @@ -435,6 +439,7 @@ static const struct i2s_soc_info 
> jz4770_i2s_soc_info = {
>  	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>  	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>  	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>  };
> 
>  static const struct i2s_soc_info jz4780_i2s_soc_info = {
> @@ -443,6 +448,7 @@ static const struct i2s_soc_info 
> jz4780_i2s_soc_info = {
>  	.field_tx_fifo_thresh	= REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>  	.field_i2sdiv_capture	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>  	.field_i2sdiv_playback	= REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> +	.pll_clk_name		= "pll half",
>  };
> 
>  static const struct snd_soc_component_driver jz4740_i2s_component = {
> --
> 2.35.1
> 

Re: [PATCH v4 08/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:41 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> The audio controller on JZ47xx SoCs can transfer 20- and 24-bit
> samples in the FIFO, so allow those formats to be used with the
> I2S driver. Although the FIFO doesn't care about the in-memory
> sample format, we only support 4-byte format variants because the
> DMA controller on these SoCs cannot transfer in 3-byte multiples.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  sound/soc/jz4740/jz4740-i2s.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index a41398c24d0e..9be2f3f1b376 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -238,9 +238,15 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  	case SNDRV_PCM_FORMAT_S8:
>  		sample_size = 0;
>  		break;
> -	case SNDRV_PCM_FORMAT_S16:
> +	case SNDRV_PCM_FORMAT_S16_LE:


I had to lookup the macro to verify, but this is correct.

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

>  		sample_size = 1;
>  		break;
> +	case SNDRV_PCM_FORMAT_S20_LE:
> +		sample_size = 3;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		sample_size = 4;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -375,7 +381,9 @@ static const struct snd_soc_dai_ops 
> jz4740_i2s_dai_ops = {
>  };
> 
>  #define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
> -		SNDRV_PCM_FMTBIT_S16_LE)
> +			 SNDRV_PCM_FMTBIT_S16_LE | \
> +			 SNDRV_PCM_FMTBIT_S20_LE | \
> +			 SNDRV_PCM_FMTBIT_S24_LE)
> 
>  static struct snd_soc_dai_driver jz4740_i2s_dai = {
>  	.probe = jz4740_i2s_dai_probe,
> --
> 2.35.1
> 

Re: [PATCH v4 10/11] ASoC: jz4740-i2s: Move component functions near the component driver

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:43 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Move the component suspend/resume functions near the definition
> of the component driver to emphasize that they're unrelated to
> the DAI functions.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

I'm not really fond of moving code like that, so I'll leave Mark with 
the liberty to take or not this patch.

Acked-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 72 
> +++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 70b9d28a40ce..5db73f12efcf 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -303,42 +303,6 @@ static int jz4740_i2s_set_sysclk(struct 
> snd_soc_dai *dai, int clk_id,
>  	return ret;
>  }
> 
> -static int jz4740_i2s_suspend(struct snd_soc_component *component)
> -{
> -	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> -
> -	if (snd_soc_component_active(component)) {
> -		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, 
> JZ_AIC_CONF_ENABLE);
> -		clk_disable_unprepare(i2s->clk_i2s);
> -	}
> -
> -	clk_disable_unprepare(i2s->clk_aic);
> -
> -	return 0;
> -}
> -
> -static int jz4740_i2s_resume(struct snd_soc_component *component)
> -{
> -	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> -	int ret;
> -
> -	ret = clk_prepare_enable(i2s->clk_aic);
> -	if (ret)
> -		return ret;
> -
> -	if (snd_soc_component_active(component)) {
> -		ret = clk_prepare_enable(i2s->clk_i2s);
> -		if (ret) {
> -			clk_disable_unprepare(i2s->clk_aic);
> -			return ret;
> -		}
> -
> -		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
> -	}
> -
> -	return 0;
> -}
> -
>  static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> @@ -459,6 +423,42 @@ static const struct i2s_soc_info 
> jz4780_i2s_soc_info = {
>  	.pll_clk_name		= "pll half",
>  };
> 
> +static int jz4740_i2s_suspend(struct snd_soc_component *component)
> +{
> +	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> +
> +	if (snd_soc_component_active(component)) {
> +		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, 
> JZ_AIC_CONF_ENABLE);
> +		clk_disable_unprepare(i2s->clk_i2s);
> +	}
> +
> +	clk_disable_unprepare(i2s->clk_aic);
> +
> +	return 0;
> +}
> +
> +static int jz4740_i2s_resume(struct snd_soc_component *component)
> +{
> +	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	ret = clk_prepare_enable(i2s->clk_aic);
> +	if (ret)
> +		return ret;
> +
> +	if (snd_soc_component_active(component)) {
> +		ret = clk_prepare_enable(i2s->clk_i2s);
> +		if (ret) {
> +			clk_disable_unprepare(i2s->clk_aic);
> +			return ret;
> +		}
> +
> +		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct snd_soc_component_driver jz4740_i2s_component = {
>  	.name			= "jz4740-i2s",
>  	.suspend		= jz4740_i2s_suspend,
> --
> 2.35.1
> 

Re: [PATCH v4 11/11] ASoC: jz4740-i2s: Refactor DAI probe/remove ops as component ops

[Paul Cercueil]

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:44 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Move most of the DAI probe/remove logic into component ops.
> This makes things more consistent because the AIC clock is
> now managed solely from the component side. And it makes it
> easier to add codec switching support later on.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 54 
> +++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index 5db73f12efcf..d99a19bc5166 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -306,32 +306,10 @@ static int jz4740_i2s_set_sysclk(struct 
> snd_soc_dai *dai, int clk_id,
>  static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -	int ret;
> -
> -	ret = clk_prepare_enable(i2s->clk_aic);
> -	if (ret)
> -		return ret;
> 
>  	snd_soc_dai_init_dma_data(dai, &i2s->playback_dma_data,
>  		&i2s->capture_dma_data);
> 
> -	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
> -
> -	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
> -		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
> -		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
> -
> -	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
> -	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
> -
> -	return 0;
> -}
> -
> -static int jz4740_i2s_dai_remove(struct snd_soc_dai *dai)
> -{
> -	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> -	clk_disable_unprepare(i2s->clk_aic);
>  	return 0;
>  }
> 
> @@ -351,7 +329,6 @@ static const struct snd_soc_dai_ops 
> jz4740_i2s_dai_ops = {
> 
>  static struct snd_soc_dai_driver jz4740_i2s_dai = {
>  	.probe = jz4740_i2s_dai_probe,
> -	.remove = jz4740_i2s_dai_remove,
>  	.playback = {
>  		.channels_min = 1,
>  		.channels_max = 2,
> @@ -389,7 +366,6 @@ static const struct i2s_soc_info 
> jz4760_i2s_soc_info = {
> 
>  static struct snd_soc_dai_driver jz4770_i2s_dai = {
>  	.probe = jz4740_i2s_dai_probe,
> -	.remove = jz4740_i2s_dai_remove,
>  	.playback = {
>  		.channels_min = 1,
>  		.channels_max = 2,
> @@ -459,8 +435,38 @@ static int jz4740_i2s_resume(struct 
> snd_soc_component *component)
>  	return 0;
>  }
> 
> +static int jz4740_i2s_probe(struct snd_soc_component *component)
> +{
> +	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	ret = clk_prepare_enable(i2s->clk_aic);
> +	if (ret)
> +		return ret;
> +
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
> +
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CONF,
> +		     JZ_AIC_CONF_OVERFLOW_PLAY_LAST |
> +		     JZ_AIC_CONF_I2S | JZ_AIC_CONF_INTERNAL_CODEC);
> +
> +	regmap_field_write(i2s->field_rx_fifo_thresh, 7);
> +	regmap_field_write(i2s->field_tx_fifo_thresh, 8);
> +
> +	return 0;
> +}
> +
> +static void jz4740_i2s_remove(struct snd_soc_component *component)
> +{
> +	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> +
> +	clk_disable_unprepare(i2s->clk_aic);
> +}
> +
>  static const struct snd_soc_component_driver jz4740_i2s_component = {
>  	.name			= "jz4740-i2s",
> +	.probe			= jz4740_i2s_probe,
> +	.remove			= jz4740_i2s_remove,
>  	.suspend		= jz4740_i2s_suspend,
>  	.resume			= jz4740_i2s_resume,
>  	.legacy_dai_naming	= 1,
> --
> 2.35.1
> 

Re: [PATCH v4 03/11] ASoC: jz4740-i2s: Convert to regmap API

[Paul Cercueil]

Hi Aidan,

I guess this patch will change if you update patch #1 with my feedback. 
So if a V5 is needed I'll review it then.

Cheers,
-Paul


Le ven., juil. 8 2022 at 17:02:36 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> Using regmap for accessing the AIC registers makes the driver a
> little easier to read, and later refactors can take advantage of
> regmap APIs to further simplify the driver.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  sound/soc/jz4740/Kconfig      |   1 +
>  sound/soc/jz4740/jz4740-i2s.c | 107 
> +++++++++++++---------------------
>  2 files changed, 40 insertions(+), 68 deletions(-)
> 
> diff --git a/sound/soc/jz4740/Kconfig b/sound/soc/jz4740/Kconfig
> index e72f826062e9..dd3b4507fbe6 100644
> --- a/sound/soc/jz4740/Kconfig
> +++ b/sound/soc/jz4740/Kconfig
> @@ -3,6 +3,7 @@ config SND_JZ4740_SOC_I2S
>  	tristate "SoC Audio (I2S protocol) for Ingenic JZ4740 SoC"
>  	depends on MIPS || COMPILE_TEST
>  	depends on HAS_IOMEM
> +	select REGMAP_MMIO
>  	select SND_SOC_GENERIC_DMAENGINE_PCM
>  	help
>  	  Say Y if you want to use I2S protocol and I2S codec on Ingenic 
> JZ4740
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index adf896333584..1393b383a886 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
> 
>  #include <linux/clk.h>
> @@ -98,7 +99,7 @@ struct i2s_soc_info {
>  };
> 
>  struct jz4740_i2s {
> -	void __iomem *base;
> +	struct regmap *regmap;
> 
>  	struct clk *clk_aic;
>  	struct clk *clk_i2s;
> @@ -109,23 +110,10 @@ struct jz4740_i2s {
>  	const struct i2s_soc_info *soc_info;
>  };
> 
> -static inline uint32_t jz4740_i2s_read(const struct jz4740_i2s *i2s,
> -	unsigned int reg)
> -{
> -	return readl(i2s->base + reg);
> -}
> -
> -static inline void jz4740_i2s_write(const struct jz4740_i2s *i2s,
> -	unsigned int reg, uint32_t value)
> -{
> -	writel(value, i2s->base + reg);
> -}
> -
>  static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
>  	struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -	uint32_t conf, ctrl;
>  	int ret;
> 
>  	/*
> @@ -133,14 +121,10 @@ static int jz4740_i2s_startup(struct 
> snd_pcm_substream *substream,
>  	 * FIFO that is starting up.
>  	 */
>  	if (!i2s->soc_info->shared_fifo_flush) {
> -		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -
>  		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
> +			regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, 
> JZ4760_AIC_CTRL_TFLUSH);
>  		else
> -			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
> -
> -		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +			regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, 
> JZ4760_AIC_CTRL_RFLUSH);
>  	}
> 
>  	if (snd_soc_dai_active(dai))
> @@ -150,20 +134,14 @@ static int jz4740_i2s_startup(struct 
> snd_pcm_substream *substream,
>  	 * When there is a shared flush bit for both FIFOs we can
>  	 * only flush the FIFOs if no other stream has started.
>  	 */
> -	if (i2s->soc_info->shared_fifo_flush) {
> -		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -		ctrl |= JZ_AIC_CTRL_FLUSH;
> -		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> -	}
> +	if (i2s->soc_info->shared_fifo_flush)
> +		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_FLUSH);
> 
>  	ret = clk_prepare_enable(i2s->clk_i2s);
>  	if (ret)
>  		return ret;
> 
> -	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
> -	conf |= JZ_AIC_CONF_ENABLE;
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> -
> +	regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
>  	return 0;
>  }
> 
> @@ -171,14 +149,11 @@ static void jz4740_i2s_shutdown(struct 
> snd_pcm_substream *substream,
>  	struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -	uint32_t conf;
> 
>  	if (snd_soc_dai_active(dai))
>  		return;
> 
> -	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
> -	conf &= ~JZ_AIC_CONF_ENABLE;
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> +	regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
> 
>  	clk_disable_unprepare(i2s->clk_i2s);
>  }
> @@ -187,8 +162,6 @@ static int jz4740_i2s_trigger(struct 
> snd_pcm_substream *substream, int cmd,
>  	struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> -	uint32_t ctrl;
>  	uint32_t mask;
> 
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> @@ -196,38 +169,30 @@ static int jz4740_i2s_trigger(struct 
> snd_pcm_substream *substream, int cmd,
>  	else
>  		mask = JZ_AIC_CTRL_ENABLE_CAPTURE | JZ_AIC_CTRL_ENABLE_RX_DMA;
> 
> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		ctrl |= mask;
> +		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> -		ctrl &= ~mask;
> +		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask);
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
> 
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> -
>  	return 0;
>  }
> 
>  static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int 
> fmt)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> -	uint32_t format = 0;
> -	uint32_t conf;
> -
> -	conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
> -
> -	conf &= ~(JZ_AIC_CONF_BIT_CLK_MASTER | JZ_AIC_CONF_SYNC_CLK_MASTER);
> +	const unsigned int conf_mask = JZ_AIC_CONF_BIT_CLK_MASTER |
> +				       JZ_AIC_CONF_SYNC_CLK_MASTER;
> +	unsigned int conf = 0, format = 0;
> 
>  	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
>  	case SND_SOC_DAIFMT_BP_FP:
> @@ -263,8 +228,8 @@ static int jz4740_i2s_set_fmt(struct snd_soc_dai 
> *dai, unsigned int fmt)
>  		return -EINVAL;
>  	}
> 
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_I2S_FMT, format);
> +	regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF, conf_mask, conf);
> +	regmap_write(i2s->regmap, JZ_REG_AIC_I2S_FMT, format);
> 
>  	return 0;
>  }
> @@ -277,9 +242,9 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  	uint32_t ctrl, div_reg;
>  	int div;
> 
> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> +	regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
> +	regmap_read(i2s->regmap, JZ_REG_AIC_CLK_DIV, &div_reg);
> 
> -	div_reg = jz4740_i2s_read(i2s, JZ_REG_AIC_CLK_DIV);
>  	div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
> 
>  	switch (params_format(params)) {
> @@ -316,8 +281,8 @@ static int jz4740_i2s_hw_params(struct 
> snd_pcm_substream *substream,
>  		}
>  	}
> 
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CLK_DIV, div_reg);
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CLK_DIV, div_reg);
> 
>  	return 0;
>  }
> @@ -354,13 +319,9 @@ static int jz4740_i2s_set_sysclk(struct 
> snd_soc_dai *dai, int clk_id,
>  static int jz4740_i2s_suspend(struct snd_soc_component *component)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> -	uint32_t conf;
> 
>  	if (snd_soc_component_active(component)) {
> -		conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
> -		conf &= ~JZ_AIC_CONF_ENABLE;
> -		jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> -
> +		regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, 
> JZ_AIC_CONF_ENABLE);
>  		clk_disable_unprepare(i2s->clk_i2s);
>  	}
> 
> @@ -372,7 +333,6 @@ static int jz4740_i2s_suspend(struct 
> snd_soc_component *component)
>  static int jz4740_i2s_resume(struct snd_soc_component *component)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> -	uint32_t conf;
>  	int ret;
> 
>  	ret = clk_prepare_enable(i2s->clk_aic);
> @@ -386,9 +346,7 @@ static int jz4740_i2s_resume(struct 
> snd_soc_component *component)
>  			return ret;
>  		}
> 
> -		conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
> -		conf |= JZ_AIC_CONF_ENABLE;
> -		jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> +		regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
>  	}
> 
>  	return 0;
> @@ -421,8 +379,8 @@ static int jz4740_i2s_dai_probe(struct 
> snd_soc_dai *dai)
>  			JZ_AIC_CONF_INTERNAL_CODEC;
>  	}
> 
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
> +	regmap_write(i2s->regmap, JZ_REG_AIC_CONF, conf);
> 
>  	return 0;
>  }
> @@ -521,11 +479,19 @@ static const struct of_device_id 
> jz4740_of_matches[] = {
>  };
>  MODULE_DEVICE_TABLE(of, jz4740_of_matches);
> 
> +static const struct regmap_config jz4740_i2s_regmap_config = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= JZ_REG_AIC_FIFO,
> +};
> +
>  static int jz4740_i2s_dev_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct jz4740_i2s *i2s;
>  	struct resource *mem;
> +	void __iomem *regs;
>  	int ret;
> 
>  	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
> @@ -534,9 +500,9 @@ static int jz4740_i2s_dev_probe(struct 
> platform_device *pdev)
> 
>  	i2s->soc_info = device_get_match_data(dev);
> 
> -	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> -	if (IS_ERR(i2s->base))
> -		return PTR_ERR(i2s->base);
> +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> 
>  	i2s->playback_dma_data.maxburst = 16;
>  	i2s->playback_dma_data.addr = mem->start + JZ_REG_AIC_FIFO;
> @@ -552,6 +518,11 @@ static int jz4740_i2s_dev_probe(struct 
> platform_device *pdev)
>  	if (IS_ERR(i2s->clk_i2s))
>  		return PTR_ERR(i2s->clk_i2s);
> 
> +	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
> +					    &jz4740_i2s_regmap_config);
> +	if (IS_ERR(i2s->regmap))
> +		return PTR_ERR(i2s->regmap);
> +
>  	platform_set_drvdata(pdev, i2s);
> 
>  	ret = devm_snd_soc_register_component(dev, &jz4740_i2s_component,
> --
> 2.35.1
> 

Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le ven., juil. 8 2022 at 17:02:34 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> On the JZ4740, there is a single bit that flushes (empties) both
>> the transmit and receive FIFO. Later SoCs have independent flush
>> bits for each FIFO, which allows us to flush the right FIFO when
>> starting up a stream.
>> This also fixes a bug: since we were only setting the JZ4740's
>> flush bit, which corresponds to the TX FIFO flush bit on other
>> SoCs, other SoCs were not having their RX FIFO flushed at all.
>> Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  sound/soc/jz4740/jz4740-i2s.c | 33 ++++++++++++++++++++++++++++++---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
>> index ecd8df70d39c..576f31f9d734 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -64,6 +64,9 @@
>>  #define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1)
>>  #define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0)
>> +#define JZ4760_AIC_CTRL_TFLUSH BIT(8)
>> +#define JZ4760_AIC_CTRL_RFLUSH BIT(7)
>
> Just rename JZ_AIC_CTRL_FLUSH to JZ_AIC_CTRL_TFLUSH and introduce
> JZ_AIC_CTRL_RLUSH.
>

According to the JZ4740 programming manual JZ_AIC_CTRL_FLUSH flushes
both FIFOs, so it's not equivalent JZ4760_AIC_CTRL_TFLUSH. I don't
think it's a good idea to confuse the two, or we'd need comments to
explain why JZ4740 uses TFLUSH but not RFLUSH.

>> +
>>  #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
>>  #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
>> @@ -90,6 +93,8 @@ enum jz47xx_i2s_version {
>>  struct i2s_soc_info {
>>  	enum jz47xx_i2s_version version;
>>  	struct snd_soc_dai_driver *dai;
>> +
>> +	bool shared_fifo_flush;
>>  };
>>  struct jz4740_i2s {
>> @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct snd_pcm_substream
>> *substream,
>>  	uint32_t conf, ctrl;
>>  	int ret;
>> +	/*
>> +	 * When we can flush FIFOs independently, only flush the
>> +	 * FIFO that is starting up.
>> +	 */
>> +	if (!i2s->soc_info->shared_fifo_flush) {
>> +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> +
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
>> +		else
>> +			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
>> +
>> +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> +	}
>
> Wouldn't it be simpler to do one single if/else? And hy is one checked before
> the (snd_soc_dai_active(dai)) check, and the other is checked after?

snd_soc_dai_active() is essentially checking if there's an active
substream. Eg. if no streams are open and you start playback, then
the DAI will be inactive. If you then start capture while playback is
running, the DAI is already active.

With a shared flush bit we can only flush if there are no other active
substreams (because we don't want to disturb the active stream by
flushing the FIFO) so it goes after the snd_soc_dai_active() check.

When the FIFOs can be separately flushed, flushing can be done before
the check because it won't disturb any active substream.

> You could do something like this:
>
> ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>
> if (i2s->soc_info->shared_fifo_flush ||
>    substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>    ctrl |= JZ_AIC_CTRL_TFLUSH;
> } else {
>    ctrl |= JZ_AIC_CTRL_RFLUSH;
> }
>
> jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>
> Cheers,
> -Paul
>

>> +
>>  	if (snd_soc_dai_active(dai))
>>  		return 0;
>> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> -	ctrl |= JZ_AIC_CTRL_FLUSH;
>> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> +	/*
>> +	 * When there is a shared flush bit for both FIFOs we can
>> +	 * only flush the FIFOs if no other stream has started.
>> +	 */
>> +	if (i2s->soc_info->shared_fifo_flush) {
>> +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> +		ctrl |= JZ_AIC_CTRL_FLUSH;
>> +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> +	}
>>  	ret = clk_prepare_enable(i2s->clk_i2s);
>>  	if (ret)
>> @@ -444,6 +470,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = {
>>  static const struct i2s_soc_info jz4740_i2s_soc_info = {
>>  	.version = JZ_I2S_JZ4740,
>>  	.dai = &jz4740_i2s_dai,
>> +	.shared_fifo_flush = true,
>>  };
>>  static const struct i2s_soc_info jz4760_i2s_soc_info = {
>> --
>> 2.35.1
>> 

Re: (subset) [PATCH v4 00/11] ASoC: cleanups and improvements for jz4740-i2s

[Mark Brown]

On Fri, 8 Jul 2022 17:02:33 +0100, Aidan MacDonald wrote:
> This series is a preparatory cleanup of the jz4740-i2s driver before
> adding support for a new SoC. The two improvements are lifting
> unnecessary restrictions on sample rates and formats -- the existing
> ones appear to be derived from the limitations of the JZ4740's internal
> codec and don't reflect the actual capabilities of the I2S controller.
> 
> I'm unable to test the series on any JZ47xx SoCs, but I have tested
> on an X1000 (which is the SoC I'll be adding in a followup series).
> 
> [...]

Applied to

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

Thanks!

[02/11] ASoC: jz4740-i2s: Remove unused 'mem' resource
        commit: cd57272c4e686d4ad2d2e775a40a3eac9f96ec7c
[04/11] ASoC: jz4740-i2s: Simplify using regmap fields
        (no commit info)
[05/11] ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback
        (no commit info)
[06/11] ASoC: jz4740-i2s: Align macro values and sort includes
        (no commit info)
[07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific
        (no commit info)
[08/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats
        (no commit info)
[09/11] ASoC: jz4740-i2s: Support continuous sample rate
        (no commit info)
[10/11] ASoC: jz4740-i2s: Move component functions near the component driver
        (no commit info)

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

Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits

[Paul Cercueil]

Hi Aidan,

Le mer., juil. 20 2022 at 15:43:06 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 
> Paul Cercueil <paul@crapouillou.net> writes:
> 
>>  Hi Aidan,
>> 
>>  Le ven., juil. 8 2022 at 17:02:34 +0100, Aidan MacDonald
>>  <aidanmacdonald.0x0@gmail.com> a écrit :
>>>  On the JZ4740, there is a single bit that flushes (empties) both
>>>  the transmit and receive FIFO. Later SoCs have independent flush
>>>  bits for each FIFO, which allows us to flush the right FIFO when
>>>  starting up a stream.
>>>  This also fixes a bug: since we were only setting the JZ4740's
>>>  flush bit, which corresponds to the TX FIFO flush bit on other
>>>  SoCs, other SoCs were not having their RX FIFO flushed at all.
>>>  Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
>>>  Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>>>  ---
>>>   sound/soc/jz4740/jz4740-i2s.c | 33 
>>> ++++++++++++++++++++++++++++++---
>>>   1 file changed, 30 insertions(+), 3 deletions(-)
>>>  diff --git a/sound/soc/jz4740/jz4740-i2s.c 
>>> b/sound/soc/jz4740/jz4740-i2s.c
>>>  index ecd8df70d39c..576f31f9d734 100644
>>>  --- a/sound/soc/jz4740/jz4740-i2s.c
>>>  +++ b/sound/soc/jz4740/jz4740-i2s.c
>>>  @@ -64,6 +64,9 @@
>>>   #define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1)
>>>   #define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0)
>>>  +#define JZ4760_AIC_CTRL_TFLUSH BIT(8)
>>>  +#define JZ4760_AIC_CTRL_RFLUSH BIT(7)
>> 
>>  Just rename JZ_AIC_CTRL_FLUSH to JZ_AIC_CTRL_TFLUSH and introduce
>>  JZ_AIC_CTRL_RLUSH.
>> 
> 
> According to the JZ4740 programming manual JZ_AIC_CTRL_FLUSH flushes
> both FIFOs, so it's not equivalent JZ4760_AIC_CTRL_TFLUSH. I don't
> think it's a good idea to confuse the two, or we'd need comments to
> explain why JZ4740 uses TFLUSH but not RFLUSH.

"shared_fifo_flush" is pretty much self-explanatory though. It then 
becomes obvious looking at the code that when this flag is set, TFLUSH 
flushes both FIFOs.

If you prefer... you can #define JZ_AIC_CTRL_FLUSH JZ_AIC_CTRL_TFLUSH. 
I don't like the JZ4760 prefix, this is in no way specific to the 
JZ4760.

> 
>>>  +
>>>   #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
>>>   #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
>>>  @@ -90,6 +93,8 @@ enum jz47xx_i2s_version {
>>>   struct i2s_soc_info {
>>>   	enum jz47xx_i2s_version version;
>>>   	struct snd_soc_dai_driver *dai;
>>>  +
>>>  +	bool shared_fifo_flush;
>>>   };
>>>   struct jz4740_i2s {
>>>  @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct 
>>> snd_pcm_substream
>>>  *substream,
>>>   	uint32_t conf, ctrl;
>>>   	int ret;
>>>  +	/*
>>>  +	 * When we can flush FIFOs independently, only flush the
>>>  +	 * FIFO that is starting up.
>>>  +	 */
>>>  +	if (!i2s->soc_info->shared_fifo_flush) {
>>>  +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>>>  +
>>>  +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>  +			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
>>>  +		else
>>>  +			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
>>>  +
>>>  +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>>>  +	}
>> 
>>  Wouldn't it be simpler to do one single if/else? And hy is one 
>> checked before
>>  the (snd_soc_dai_active(dai)) check, and the other is checked after?
> 
> snd_soc_dai_active() is essentially checking if there's an active
> substream. Eg. if no streams are open and you start playback, then
> the DAI will be inactive. If you then start capture while playback is
> running, the DAI is already active.
> 
> With a shared flush bit we can only flush if there are no other active
> substreams (because we don't want to disturb the active stream by
> flushing the FIFO) so it goes after the snd_soc_dai_active() check.
> 
> When the FIFOs can be separately flushed, flushing can be done before
> the check because it won't disturb any active substream.

Ok. It makes sense then. Please add some info about this in the commit 
message, because it really wasn't obvious to me.

You should maybe factorize the read-modify-write into its own function. 
I know this gets eventually modified by [03/11], but this [01/11] is a 
bugfix so it will be applied to older kernels, and I'd rather not have 
duplicated code there.

Cheers,
-Paul

> 
>>  You could do something like this:
>> 
>>  ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> 
>>  if (i2s->soc_info->shared_fifo_flush ||
>>     substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>     ctrl |= JZ_AIC_CTRL_TFLUSH;
>>  } else {
>>     ctrl |= JZ_AIC_CTRL_RFLUSH;
>>  }
>> 
>>  jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> 
>>  Cheers,
>>  -Paul
>> 
> 
>>>  +
>>>   	if (snd_soc_dai_active(dai))
>>>   		return 0;
>>>  -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>>>  -	ctrl |= JZ_AIC_CTRL_FLUSH;
>>>  -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>>>  +	/*
>>>  +	 * When there is a shared flush bit for both FIFOs we can
>>>  +	 * only flush the FIFOs if no other stream has started.
>>>  +	 */
>>>  +	if (i2s->soc_info->shared_fifo_flush) {
>>>  +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>>>  +		ctrl |= JZ_AIC_CTRL_FLUSH;
>>>  +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>>>  +	}
>>>   	ret = clk_prepare_enable(i2s->clk_i2s);
>>>   	if (ret)
>>>  @@ -444,6 +470,7 @@ static struct snd_soc_dai_driver 
>>> jz4740_i2s_dai = {
>>>   static const struct i2s_soc_info jz4740_i2s_soc_info = {
>>>   	.version = JZ_I2S_JZ4740,
>>>   	.dai = &jz4740_i2s_dai,
>>>  +	.shared_fifo_flush = true,
>>>   };
>>>   static const struct i2s_soc_info jz4760_i2s_soc_info = {
>>>  --
>>>  2.35.1
>>> 
> 

Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le mer., juil. 20 2022 at 15:43:06 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> Paul Cercueil <paul@crapouillou.net> writes:
>>
>> According to the JZ4740 programming manual JZ_AIC_CTRL_FLUSH flushes
>> both FIFOs, so it's not equivalent JZ4760_AIC_CTRL_TFLUSH. I don't
>> think it's a good idea to confuse the two, or we'd need comments to
>> explain why JZ4740 uses TFLUSH but not RFLUSH.
>
> "shared_fifo_flush" is pretty much self-explanatory though. It then becomes
> obvious looking at the code that when this flag is set, TFLUSH flushes both
> FIFOs.
>
> If you prefer... you can #define JZ_AIC_CTRL_FLUSH JZ_AIC_CTRL_TFLUSH. I don't
> like the JZ4760 prefix, this is in no way specific to the JZ4760.
>

Makes sense, I'll stick with TFLUSH / RFLUSH only.

>>
>>>>  +
>>>>   #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
>>>>   #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
>>>>  @@ -90,6 +93,8 @@ enum jz47xx_i2s_version {
>>>>   struct i2s_soc_info {
>>>>   	enum jz47xx_i2s_version version;
>>>>   	struct snd_soc_dai_driver *dai;
>>>>  +
>>>>  +	bool shared_fifo_flush;
>>>>   };
>>>>   struct jz4740_i2s {
>>>>  @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct
>>>> snd_pcm_substream
>>>>  *substream,
>>>>   	uint32_t conf, ctrl;
>>>>   	int ret;
>>>>  +	/*
>>>>  +	 * When we can flush FIFOs independently, only flush the
>>>>  +	 * FIFO that is starting up.
>>>>  +	 */
>>>>  +	if (!i2s->soc_info->shared_fifo_flush) {
>>>>  +		ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>>>>  +
>>>>  +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>>  +			ctrl |= JZ4760_AIC_CTRL_TFLUSH;
>>>>  +		else
>>>>  +			ctrl |= JZ4760_AIC_CTRL_RFLUSH;
>>>>  +
>>>>  +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>>>>  +	}
>>>  Wouldn't it be simpler to do one single if/else? And hy is one checked
>>> before
>>>  the (snd_soc_dai_active(dai)) check, and the other is checked after?
>> snd_soc_dai_active() is essentially checking if there's an active
>> substream. Eg. if no streams are open and you start playback, then
>> the DAI will be inactive. If you then start capture while playback is
>> running, the DAI is already active.
>> With a shared flush bit we can only flush if there are no other active
>> substreams (because we don't want to disturb the active stream by
>> flushing the FIFO) so it goes after the snd_soc_dai_active() check.
>> When the FIFOs can be separately flushed, flushing can be done before
>> the check because it won't disturb any active substream.
>
> Ok. It makes sense then. Please add some info about this in the commit message,
> because it really wasn't obvious to me.

It wasn't that obvious to me either :)

I've added code comments too since it seems likely to trip people up
if you're only taking a casual glance.

> You should maybe factorize the read-modify-write into its own function. I know
> this gets eventually modified by [03/11], but this [01/11] is a bugfix so it
> will be applied to older kernels, and I'd rather not have duplicated code
> there.
>
> Cheers,
> -Paul

And I've factored out the r-m-w helper as requested.

Regards,
Aidan

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Aidan MacDonald]

Zhou Yanjie <zhouyu@wanyeetech.com> writes:

> Hi Paul,
>
> On 2022/7/13 下午11:07, Paul Cercueil wrote:
>> Hi Zhou,
>>
>> Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie <zhouyu@wanyeetech.com>
>> a écrit :
>>> Hi Aidan,
>>>
>>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>> @@ -400,6 +402,7 @@ static const struct i2s_soc_info jz4740_i2s_soc_info =
>>>> {
>>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>> +    .pll_clk_name        = "pll half",
>>>>       .shared_fifo_flush    = true,
>>>>   };
>>>
>>>
>>> Since JZ4760, according to the description of the I2SCDR register,
>>> Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
>>> so it seems also inappropriate to use "pll half" for these SoCs.
>>
>> The device tree passes the clock as "pll half". So the driver should use this
>> name as well...
>
>
> I see...
>
> It seems that the device tree of JZ4770 has used "pll half" already,
> but there is no "pll half" used anywhere in the device tree of JZ4780,
> maybe we can keep the pll_clk_name of JZ4770 as "pll half", and change
> the pll_clk_name of JZ4780 to a more reasonable name.
>
>
> Thanks and best regards!

Actually, the clock names in the DT are meaningless. The clk_get() call
matches only the clock's name in the CGU driver. So in fact the driver
is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
it has no "pll half", and three possible parents for its "i2s" clock.

Since the driver only supports the internal codec, which requires the
"ext" clock, there isn't a problem in practice.

I'm just going to drop this patch and leave .set_sysclk() alone for now.
I think a better approach is to have the DT define an array of parent
clocks for .set_sysclk()'s use, instead of hardcoding parents in the
driver. If the parent array is missing the driver can default to using
"ext" so existing DTs will work.

Regards,
Aidan

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

Hi Aidan,

Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 
> Zhou Yanjie <zhouyu@wanyeetech.com> writes:
> 
>>  Hi Paul,
>> 
>>  On 2022/7/13 下午11:07, Paul Cercueil wrote:
>>>  Hi Zhou,
>>> 
>>>  Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie 
>>> <zhouyu@wanyeetech.com>
>>>  a écrit :
>>>>  Hi Aidan,
>>>> 
>>>>  On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>>  @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
>>>>> jz4740_i2s_soc_info =
>>>>>  {
>>>>>        .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 
>>>>> 11),
>>>>>        .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 
>>>>> 0, 3),
>>>>>        .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 
>>>>> 0, 3),
>>>>>  +    .pll_clk_name        = "pll half",
>>>>>        .shared_fifo_flush    = true,
>>>>>    };
>>>> 
>>>> 
>>>>  Since JZ4760, according to the description of the I2SCDR register,
>>>>  Ingenic SoCs no longer use PLL/2 clock, but directly use PLL 
>>>> clock,
>>>>  so it seems also inappropriate to use "pll half" for these SoCs.
>>> 
>>>  The device tree passes the clock as "pll half". So the driver 
>>> should use this
>>>  name as well...
>> 
>> 
>>  I see...
>> 
>>  It seems that the device tree of JZ4770 has used "pll half" already,
>>  but there is no "pll half" used anywhere in the device tree of 
>> JZ4780,
>>  maybe we can keep the pll_clk_name of JZ4770 as "pll half", and 
>> change
>>  the pll_clk_name of JZ4780 to a more reasonable name.
>> 
>> 
>>  Thanks and best regards!
> 
> Actually, the clock names in the DT are meaningless. The clk_get() 
> call
> matches only the clock's name in the CGU driver. So in fact the driver
> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
> it has no "pll half", and three possible parents for its "i2s" clock.

That's not true. The clock names are matched via DT.

Only in the case where a corresponding clock cannot be found via DT 
will it search for the clock name among the clock providers. I believe 
this is a legacy mechanism and you absolutely shouldn't rely on it.

-Paul

> Since the driver only supports the internal codec, which requires the
> "ext" clock, there isn't a problem in practice.
> 
> I'm just going to drop this patch and leave .set_sysclk() alone for 
> now.
> I think a better approach is to have the DT define an array of parent
> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
> driver. If the parent array is missing the driver can default to using
> "ext" so existing DTs will work.
> 
> Regards,
> Aidan

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> Zhou Yanjie <zhouyu@wanyeetech.com> writes:
>>
>>>  Hi Paul,
>>>  On 2022/7/13 下午11:07, Paul Cercueil wrote:
>>>>  Hi Zhou,
>>>>  Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie
>>>> <zhouyu@wanyeetech.com>
>>>>  a écrit :
>>>>>  Hi Aidan,
>>>>>  On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>>>  @@ -400,6 +402,7 @@ static const struct i2s_soc_info jz4740_i2s_soc_info
>>>>>> =
>>>>>>  {
>>>>>>        .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>>>>>>        .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>>>>        .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>>>>  +    .pll_clk_name        = "pll half",
>>>>>>        .shared_fifo_flush    = true,
>>>>>>    };
>>>>>  Since JZ4760, according to the description of the I2SCDR register,
>>>>>  Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
>>>>>  so it seems also inappropriate to use "pll half" for these SoCs.
>>>>  The device tree passes the clock as "pll half". So the driver should use
>>>> this
>>>>  name as well...
>>>  I see...
>>>  It seems that the device tree of JZ4770 has used "pll half" already,
>>>  but there is no "pll half" used anywhere in the device tree of JZ4780,
>>>  maybe we can keep the pll_clk_name of JZ4770 as "pll half", and change
>>>  the pll_clk_name of JZ4780 to a more reasonable name.
>>>  Thanks and best regards!
>> Actually, the clock names in the DT are meaningless. The clk_get() call
>> matches only the clock's name in the CGU driver. So in fact the driver
>> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
>> it has no "pll half", and three possible parents for its "i2s" clock.
>
> That's not true. The clock names are matched via DT.
>
> Only in the case where a corresponding clock cannot be found via DT will it
> search for the clock name among the clock providers. I believe this is a legacy
> mechanism and you absolutely shouldn't rely on it.
>
> -Paul
>

What you say is only true for clk_get() with a device argument. When the
device argument is NULL -- which is the case in .set_sysclk() -- then
the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
When the dev_id is NULL, it will not match any lookup entries with a
non-null dev_id, and I believe dev_id is the mechanism that implements
DT clock lookup. Only the wildcard entries from the CGU driver will be
matched if dev_id is NULL, so the DT is being ignored.

If you don't believe me, try changing "pll half" in the device tree and
the I2S driver to something else. I have done this, and it doesn't work.
That proves the name in the device tree is not being used.

I agree we shouldn't rely on this, it's a legacy behavior, but the fact
is that's how the driver already works. I'm dropping this patch because
the driver is wrong and needs a different fix...

>> I think a better approach is to have the DT define an array of parent
>> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
>> driver. If the parent array is missing the driver can default to using
>> "ext" so existing DTs will work.

As much as I like this idea there doesn't seem to be a mechanism for
handling a free-floating array of clocks in the DT. Everything has
to be put in the main "clocks" array. That makes it pretty hard to
figure out which ones are meant to be the parent clocks.

Do you know of any way to do this generically from the DT? If there's
no way to get away from a hardcoded array of names in the driver, I can
at least add a device argument to clk_get() so it'll use the DT names.

Regards,
Aidan

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

Hi Aidan,

Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 
> Paul Cercueil <paul@crapouillou.net> writes:
> 
>>  Hi Aidan,
>> 
>>  Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
>>  <aidanmacdonald.0x0@gmail.com> a écrit :
>>>  Zhou Yanjie <zhouyu@wanyeetech.com> writes:
>>> 
>>>>   Hi Paul,
>>>>   On 2022/7/13 下午11:07, Paul Cercueil wrote:
>>>>>   Hi Zhou,
>>>>>   Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie
>>>>>  <zhouyu@wanyeetech.com>
>>>>>   a écrit :
>>>>>>   Hi Aidan,
>>>>>>   On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>>>>   @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
>>>>>>> jz4740_i2s_soc_info
>>>>>>>  =
>>>>>>>   {
>>>>>>>         .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 
>>>>>>> 8, 11),
>>>>>>>         .field_i2sdiv_capture    = 
>>>>>>> REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>>>>>         .field_i2sdiv_playback    = 
>>>>>>> REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>>>>>   +    .pll_clk_name        = "pll half",
>>>>>>>         .shared_fifo_flush    = true,
>>>>>>>     };
>>>>>>   Since JZ4760, according to the description of the I2SCDR 
>>>>>> register,
>>>>>>   Ingenic SoCs no longer use PLL/2 clock, but directly use PLL 
>>>>>> clock,
>>>>>>   so it seems also inappropriate to use "pll half" for these 
>>>>>> SoCs.
>>>>>   The device tree passes the clock as "pll half". So the driver 
>>>>> should use
>>>>>  this
>>>>>   name as well...
>>>>   I see...
>>>>   It seems that the device tree of JZ4770 has used "pll half" 
>>>> already,
>>>>   but there is no "pll half" used anywhere in the device tree of 
>>>> JZ4780,
>>>>   maybe we can keep the pll_clk_name of JZ4770 as "pll half", and 
>>>> change
>>>>   the pll_clk_name of JZ4780 to a more reasonable name.
>>>>   Thanks and best regards!
>>>  Actually, the clock names in the DT are meaningless. The clk_get() 
>>> call
>>>  matches only the clock's name in the CGU driver. So in fact the 
>>> driver
>>>  is "broken" for jz4780. It seems jz4770 doesn't work correctly 
>>> either,
>>>  it has no "pll half", and three possible parents for its "i2s" 
>>> clock.
>> 
>>  That's not true. The clock names are matched via DT.
>> 
>>  Only in the case where a corresponding clock cannot be found via DT 
>> will it
>>  search for the clock name among the clock providers. I believe this 
>> is a legacy
>>  mechanism and you absolutely shouldn't rely on it.
>> 
>>  -Paul
>> 
> 
> What you say is only true for clk_get() with a device argument. When 
> the
> device argument is NULL -- which is the case in .set_sysclk() -- then
> the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
> When the dev_id is NULL, it will not match any lookup entries with a
> non-null dev_id, and I believe dev_id is the mechanism that implements
> DT clock lookup. Only the wildcard entries from the CGU driver will be
> matched if dev_id is NULL, so the DT is being ignored.
> 
> If you don't believe me, try changing "pll half" in the device tree 
> and
> the I2S driver to something else. I have done this, and it doesn't 
> work.
> That proves the name in the device tree is not being used.

Well, let's pass them a device pointer then.

> I agree we shouldn't rely on this, it's a legacy behavior, but the 
> fact
> is that's how the driver already works. I'm dropping this patch 
> because
> the driver is wrong and needs a different fix...

"How the driver already works" is a bit misleading, I never saw this 
.set_sysclk() callback being called, so I can't really say that it 
works.

>>>  I think a better approach is to have the DT define an array of 
>>> parent
>>>  clocks for .set_sysclk()'s use, instead of hardcoding parents in 
>>> the
>>>  driver. If the parent array is missing the driver can default to 
>>> using
>>>  "ext" so existing DTs will work.
> 
> As much as I like this idea there doesn't seem to be a mechanism for
> handling a free-floating array of clocks in the DT. Everything has
> to be put in the main "clocks" array. That makes it pretty hard to
> figure out which ones are meant to be the parent clocks.
> 
> Do you know of any way to do this generically from the DT? If there's
> no way to get away from a hardcoded array of names in the driver, I 
> can
> at least add a device argument to clk_get() so it'll use the DT names.

In jz4740_i2s_set_sysclk():

#define JZ4740_I2S_FIRST_PARENT_CLK 2
parent = of_clk_get(dev->of_node, JZ4740_I2S_FIRST_PARENT_CLK + clk_id);

is how I'd do it.

The DTs all have "aic", "i2s" as the first two clocks. It is even 
enforced in the DT schemas.

Cheers,
-Paul

Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> Paul Cercueil <paul@crapouillou.net> writes:
>>
>>>  Hi Aidan,
>>>  Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
>>>  <aidanmacdonald.0x0@gmail.com> a écrit :
>>>>  Actually, the clock names in the DT are meaningless. The clk_get() call
>>>>  matches only the clock's name in the CGU driver. So in fact the driver
>>>>  is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
>>>>  it has no "pll half", and three possible parents for its "i2s" clock.
>>>  That's not true. The clock names are matched via DT.
>>>  Only in the case where a corresponding clock cannot be found via DT will it
>>>  search for the clock name among the clock providers. I believe this is a
>>> legacy
>>>  mechanism and you absolutely shouldn't rely on it.
>>>  -Paul
>>>
>> What you say is only true for clk_get() with a device argument. When the
>> device argument is NULL -- which is the case in .set_sysclk() -- then
>> the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
>> When the dev_id is NULL, it will not match any lookup entries with a
>> non-null dev_id, and I believe dev_id is the mechanism that implements
>> DT clock lookup. Only the wildcard entries from the CGU driver will be
>> matched if dev_id is NULL, so the DT is being ignored.
>> If you don't believe me, try changing "pll half" in the device tree and
>> the I2S driver to something else. I have done this, and it doesn't work.
>> That proves the name in the device tree is not being used.
>
> Well, let's pass them a device pointer then.
>

Yes, I'll do that when I revise the patch.

>> I agree we shouldn't rely on this, it's a legacy behavior, but the fact
>> is that's how the driver already works. I'm dropping this patch because
>> the driver is wrong and needs a different fix...
>
> "How the driver already works" is a bit misleading, I never saw this
> .set_sysclk() callback being called, so I can't really say that it works.
>
>>>>  I think a better approach is to have the DT define an array of parent
>>>>  clocks for .set_sysclk()'s use, instead of hardcoding parents in the
>>>>  driver. If the parent array is missing the driver can default to using
>>>>  "ext" so existing DTs will work.
>> As much as I like this idea there doesn't seem to be a mechanism for
>> handling a free-floating array of clocks in the DT. Everything has
>> to be put in the main "clocks" array. That makes it pretty hard to
>> figure out which ones are meant to be the parent clocks.
>> Do you know of any way to do this generically from the DT? If there's
>> no way to get away from a hardcoded array of names in the driver, I can
>> at least add a device argument to clk_get() so it'll use the DT names.
>
> In jz4740_i2s_set_sysclk():
>
> #define JZ4740_I2S_FIRST_PARENT_CLK 2
> parent = of_clk_get(dev->of_node, JZ4740_I2S_FIRST_PARENT_CLK + clk_id);
>
> is how I'd do it.
>
> The DTs all have "aic", "i2s" as the first two clocks. It is even enforced in
> the DT schemas.
>
> Cheers,
> -Paul

Sounds like a plan. I was hoping to avoid adding CONFIG_OF back
considering I removed it in an earlier patch since it was unused. :)

Guess it doesn't really matter for this driver since the Ingenic
SoCs need CONFIG_OF anyway.

Regards,
Aidan