Message-ID:
 <NTZPR01MB0956BFADB4B3DA507D938F669F35A@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn>
Date: Tue, 26 Mar 2024 02:02:59 +0000
From: Xingyu Wu <xingyu.wu@...rfivetech.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>, Liam Girdwood
	<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Claudiu Beznea
	<Claudiu.Beznea@...rochip.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi
 Iwai <tiwai@...e.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor.dooley@...rochip.com>
CC: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>
Subject:
 回复: [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

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

Powered by blists - more mailing lists