> > 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
Will fix. Thanks.
>
> > + 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?
Will fix.
>
> > + */
> > +
> > +#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.
Will fix.
>
> > +#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?
This is to see if the new data is divisible by period_size and to determine whether
it is enough for a period_size in the later loop.
>
> > + 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' ?
I think I just support S16_LE and S32_LE in the snd_soc_dai_driver struct,
and ALSA device will filter out other formats for me, so I didn't add them.
Do I still have to add the other case?
> > +
> > + 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?
Will fix.
>
> > +{
> > + 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?
Will add.
>
> > +}
> > +
> > +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?
Will add the test.
>
> > + 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?
Will fix.
>
> > + }
> > +
> > + 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.
OK, I will move it out of the loop. Thanks.
>
> > +
> > + 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?
Will fix.
>
> > +
> > + 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.
Will fix.
>
> > + 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?
I think both are OK.
>
> > +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?
Since I have enabled clocks before, and the device is in the suspend state after
pm_runtime_enable(), I need to disable clocks in cdns_i2s_runtime_suspend()
to match the suspend state.
>
> > +
> > + 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...
I think the clocks need to be always enabled after probe if disable pm_runtime,
and should be disabled when remove. This will do that.
>
> > +
> > + return 0;
> > +}
> > +
Best regards,
Xingyu Wu
