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

From: Pierre-Louis Bossart
Date: Wed Mar 20 2024 - 11:00:45 EST

Next message: Thorsten Blum: "Re: [PATCH] tracing: Explicitly cast divisor to fix Coccinelle warning"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 1/2] perf: Suggest inbuilt commands for unknown command"
In reply to: Xingyu Wu: "[PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller"
Next in thread: Mark Brown: "Re: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> 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;
> +}
> +

Next message: Thorsten Blum: "Re: [PATCH] tracing: Explicitly cast divisor to fix Coccinelle warning"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 1/2] perf: Suggest inbuilt commands for unknown command"
In reply to: Xingyu Wu: "[PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller"
Next in thread: Mark Brown: "Re: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]