Re: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig new file mode 100644 index 000000000000..61ef718ebfe7 --- /dev/null +++ b/sound/soc/cdns/Kconfig @@ -0,0 +1,18 @@ +# SPDX-License-Identifier: GPL-2.0-only +config SND_SOC_CADENCE_I2S_MC

•    tristate "Cadence I2S Multi-Channel Controller Device Driver"
  

• depends on HAVE_CLK

indentation is off

•    select SND_SOC_GENERIC_DMAENGINE_PCM
  

•    help
  

•     Say Y or M if you want to add support for I2S driver for the
  

•     Cadence Multi-Channel I2S Controller device. The device supports
  

•     up to a maximum of 8 channels each for play and record.
  

+// SPDX-License-Identifier: GPL-2.0 +/*

• • Cadence Multi-Channel I2S controller PCM driver
• • Copyright (c) 2022-2023 StarFive Technology Co., Ltd.

2024?

• */

+#include <linux/io.h> +#include <linux/rcupdate.h> +#include <sound/pcm_params.h> +#include "cdns-i2s-mc.h"

+#define PERIOD_BYTES_MIN 4096 +#define BUFFER_BYTES_MAX (3 * 2 * 8 * PERIOD_BYTES_MIN)

what are those 3 and 2 and 8 factors? a comment or the use of macros would help.

+#define PERIODS_MIN 2

+static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,

• 		    struct snd_pcm_runtime *runtime,
  

• 		    unsigned int tx_ptr, bool *period_elapsed,
  

• 		    snd_pcm_format_t format)
  

+{

• unsigned int period_pos = tx_ptr % runtime->period_size;

not following what the modulo is for, usually it's modulo the buffer size?

• const u16 (*p16)[2] = (void *)runtime->dma_area;
• const u32 (*p32)[2] = (void *)runtime->dma_area;
• u32 data[2];
• int i;
• for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {

• if (format == SNDRV_PCM_FORMAT_S16_LE) {
  

• 	data[0] = p16[tx_ptr][0];
  

• 	data[1] = p16[tx_ptr][1];
  

• } else if (format == SNDRV_PCM_FORMAT_S32_LE) {
  

• 	data[0] = p32[tx_ptr][0];
  

• 	data[1] = p32[tx_ptr][1];
  

• }
  

what about other formats implied by the use of 'else if' ?

• iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
  

• iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
  

• period_pos++;
  

• if (++tx_ptr >= runtime->buffer_size)
  

• 	tx_ptr = 0;
  

• }
• *period_elapsed = period_pos >= runtime->period_size;
• return tx_ptr;

+}

+static void cdns_i2s_pcm_transfer(struct cdns_i2s_dev *dev, bool push)

'push' really means 'tx' so what not use a simpler naming?

+{

• struct snd_pcm_substream *substream;
• bool active, period_elapsed;
• rcu_read_lock();
• if (push)

• substream = rcu_dereference(dev->tx_substream);
  

• else

• substream = rcu_dereference(dev->rx_substream);
  

• active = substream && snd_pcm_running(substream);
• if (active) {

• unsigned int ptr;
  

• unsigned int new_ptr;
  

• if (push) {
  

• 	ptr = READ_ONCE(dev->tx_ptr);
  

• 	new_ptr = dev->tx_fn(dev, substream->runtime, ptr,
  

• 			     &period_elapsed, dev->format);
  

• 	cmpxchg(&dev->tx_ptr, ptr, new_ptr);
  

• } else {
  

• 	ptr = READ_ONCE(dev->rx_ptr);
  

• 	new_ptr = dev->rx_fn(dev, substream->runtime, ptr,
  

• 			     &period_elapsed, dev->format);
  

• 	cmpxchg(&dev->rx_ptr, ptr, new_ptr);
  

• }
  

• if (period_elapsed)
  

• 	snd_pcm_period_elapsed(substream);
  

• }
• rcu_read_unlock();

+}

+void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev) +{

• cdns_i2s_pcm_transfer(dev, true);

+}

+void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev) +{

• cdns_i2s_pcm_transfer(dev, false);

+}

+static int cdns_i2s_pcm_open(struct snd_soc_component *component,

• 	     struct snd_pcm_substream *substream)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
• struct cdns_i2s_dev *dev = snd_soc_dai_get_drvdata(snd_soc_rtd_to_cpu(rtd, 0));
• snd_soc_set_runtime_hwparams(substream, &cdns_i2s_pcm_hardware);
• snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
• runtime->private_data = dev;
• return 0;

+}

+static int cdns_i2s_pcm_close(struct snd_soc_component *component,

• 	      struct snd_pcm_substream *substream)
  

+{

• synchronize_rcu();
• return 0;

runtime->private_data = NULL?

+}

+static int cdns_i2s_pcm_hw_params(struct snd_soc_component *component,

• 		  struct snd_pcm_substream *substream,
  

• 		  struct snd_pcm_hw_params *hw_params)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• struct cdns_i2s_dev *dev = runtime->private_data;
• dev->format = params_format(hw_params);

don't you need to test if the formats are supported?

• dev->tx_fn = cdns_i2s_pcm_tx;
• dev->rx_fn = cdns_i2s_pcm_rx;
• return 0;

+}

+static int cdns_i2s_start(struct cdns_i2s_dev *i2s,

• 	  struct snd_pcm_substream *substream)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• unsigned char max_ch = i2s->max_channels;
• unsigned char i2s_ch;
• int i;
• /* Each channel is stereo */
• i2s_ch = runtime->channels / 2;
• if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {

• if ((i2s_ch + i2s->tx_using_channels) > max_ch) {
  

• 	dev_err(i2s->dev,
  

• 		"Max %d channels: using %d for TX, do not support %d for RX\n",
  

• 		max_ch, i2s->tx_using_channels, i2s_ch);
  

• 	return -ENOMEM;
  

ENOMEM is for memory allocation, that looks more like EINVAL?

• }
  

• i2s->rx_using_channels = i2s_ch;
  

• /* Enable channels from 0 to 'max_ch' as tx */
  

• for (i = 0; i < i2s_ch; i++)
  

• 	cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
  

• 			       CDNS_I2S_TC_RECEIVER);
  

• } else {

• if ((i2s_ch + i2s->rx_using_channels) > max_ch) {
  

• 	dev_err(i2s->dev,
  

• 		"Max %d channels: using %d for RX, do not support %d for TX\n",
  

• 		max_ch, i2s->rx_using_channels, i2s_ch);
  

• 	return -ENOMEM;
  

• }
  

• i2s->tx_using_channels = i2s_ch;
  

• /* Enable channels from 'max_ch' to 0 as rx */
  

• for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
  

• 	if (i < 0)
  

• 		return -EINVAL;
  

that is a test you can probably factor out of the loop before doing anything that's inconsistent.

• 	cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
  

• 			       CDNS_I2S_TC_TRANSMITTER);
  

• }
  

• }
• cdns_i2s_enable_clock(i2s, substream->stream);
• if (i2s->irq >= 0)

• cdns_i2s_set_all_irq_mask(i2s, false);
  

• cdns_i2s_clear_int(i2s);
• return 0;

+}

+static int cdns_i2s_stop(struct cdns_i2s_dev *i2s,

• 	 struct snd_pcm_substream *substream)
  

+{

• unsigned char i2s_ch;
• int i;
• cdns_i2s_clear_int(i2s);
• if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {

• i2s_ch = i2s->rx_using_channels;
  

• for (i = 0; i < i2s_ch; i++)
  

• 	cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
  

• i2s->rx_using_channels = 0;
  

• } else {

• unsigned char max_ch = i2s->max_channels;
  

• i2s_ch = i2s->tx_using_channels;
  

• for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
  

• 	if (i < 0)
  

• 		return -EINVAL;
  

same here, first test if the channel maps are valid, then do the loop?

• 	cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
  

• }
  

• i2s->tx_using_channels = 0;
  

• }
• if (i2s->irq >= 0 && !i2s->tx_using_channels && !i2s->rx_using_channels)

• cdns_i2s_set_all_irq_mask(i2s, true);
  

• return 0;

+}

+static int cdns_i2s_set_fmt(struct snd_soc_dai *cpu_dai,

• 	    unsigned int fmt)
  

+{

• struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(cpu_dai);
• int ret = 0;
• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
• case SND_SOC_DAIFMT_CBM_CFM:

• i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
  

• i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
  

• cdns_i2s_set_ms_mode(i2s);
  

• break;
  

• case SND_SOC_DAIFMT_CBS_CFS:

• i2s->tx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
  

• i2s->rx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
  

• cdns_i2s_set_ms_mode(i2s);
  

• break;
  

• case SND_SOC_DAIFMT_CBM_CFS:
• case SND_SOC_DAIFMT_CBS_CFM:

that's the old stuff, use CBP/CBC macros please.

• ret = -EINVAL;
  

• break;
  

• default:

• dev_dbg(i2s->dev, "Invalid master/slave format\n");
  

• ret = -EINVAL;
  

• break;
  

• }
• return ret;

+}

+#ifdef CONFIG_PM

Do we need this or just rely on __unused?

+static int cdns_i2s_runtime_suspend(struct device *dev) +{

• struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
• clk_disable_unprepare(i2s->clks[1].clk); /* ICG clock */
• return 0;

+}

+static int cdns_i2s_runtime_resume(struct device *dev) +{

• struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
• return clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */

+} +#endif

+static int cdns_i2s_probe(struct platform_device *pdev) +{

• struct cdns_i2s_dev *i2s;
• struct resource *res;
• int ret;
• i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
• if (!i2s) {

• ret = -ENOMEM;
  

• goto err;
  

• }
• platform_set_drvdata(pdev, i2s);
• i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
• if (IS_ERR(i2s->base)) {

• ret = PTR_ERR(i2s->base);
  

• goto err;
  

• }
• i2s->dev = &pdev->dev;
• i2s->phybase = res->start;
• ret = cdns_i2s_init(i2s);
• if (ret)

• goto err;
  

• i2s->irq = platform_get_irq(pdev, 0);
• if (i2s->irq >= 0) {

• ret = devm_request_irq(&pdev->dev, i2s->irq, cdns_i2s_irq_handler,
  

• 		       0, pdev->name, i2s);
  

• if (ret < 0) {
  

• 	dev_err(&pdev->dev, "request_irq failed\n");
  

• 	goto err;
  

• }
  

• }
• ret = devm_snd_soc_register_component(&pdev->dev,

• 			      &cdns_i2s_component,
  

• 			      &cdns_i2s_dai, 1);
  

• if (ret < 0) {

• dev_err(&pdev->dev, "couldn't register component\n");
  

• goto err;
  

• }
• if (i2s->irq >= 0)

• ret = cdns_i2s_pcm_register(pdev);
  

• else

• ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
  

• if (ret) {

• dev_err(&pdev->dev, "could not register pcm: %d\n", ret);
  

• goto err;
  

• }
• pm_runtime_enable(&pdev->dev);
• if (pm_runtime_enabled(&pdev->dev))

• cdns_i2s_runtime_suspend(&pdev->dev);
  

that sequence looks suspicious.... Why would you suspend immediately during the probe? You're probably missing all the autosuspend stuff?

• dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",

• i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
  

• return 0;

+err:

• return ret;

+}

+static int cdns_i2s_remove(struct platform_device *pdev) +{

• pm_runtime_disable(&pdev->dev);
• if (!pm_runtime_status_suspended(&pdev->dev))

• cdns_i2s_runtime_suspend(&pdev->dev);
  

... and this one too. Once you've disabled pm_runtime, checking the status is irrelevant...

• return 0;

+}