Received-SPF: pass (google.com: domain of linux-kernel+bounces-107750-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9d123584-1feb-404b-890f-2da694cf56d5@gmail.com>
Date: Tue, 19 Mar 2024 17:57:57 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 07/13] ASoC: ti: davinci-i2s: Add TDM support
To: Bastien Curutchet <bastien.curutchet@bootlin.com>,
 Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
 Takashi Iwai <tiwai@suse.com>
Cc: linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
 Thomas Petazzoni <thomas.petazzoni@bootlin.com>, herve.codina@bootlin.com,
 christophercordahi@nanometrics.ca
References: <20240315112745.63230-1-bastien.curutchet@bootlin.com>
 <20240315112745.63230-8-bastien.curutchet@bootlin.com>
Content-Language: en-US
From: =?UTF-8?Q?P=C3=A9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>
In-Reply-To: <20240315112745.63230-8-bastien.curutchet@bootlin.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 15/03/2024 13:27, Bastien Curutchet wrote:
> TDM is not supported by the McBSP driver. The McBSP datasheet does not
> name explicitly TDM as a supported format but it is possible to configure
> the McBSP to do TDM if all slots are used by McBSP.
> 
> Add TDM support. It uses single-phase frame. Slot width is used to
> compute the McBSP's word length.
> 
> Implement the set_tdm_slot() hook of snd_soc_dai_ops struct. It only
> supports TDM if all slots are used by McBSP.
> 
> The snd_soc_dai_driver's capture.channels_max is updated from 2 to 128.
> 128 is the maximum frame length when using single-phase frame.
> playback.channels_max has not been updated as I could not test TDM on
> playbacks with my hardware.
> 
> This was tested on platform designed off of DAVINCI/OMAP_L138 with BP_FC
> format so this is only supported format for TDM yet. A check is done in
> davinci_i2s_set_dai_fmt() to prevent TDM to be used with other formats
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  sound/soc/ti/davinci-i2s.c | 81 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
> index f4514d4dff07..4adaed010700 100644
> --- a/sound/soc/ti/davinci-i2s.c
> +++ b/sound/soc/ti/davinci-i2s.c
> @@ -160,6 +160,9 @@ struct davinci_mcbsp_dev {
>  	unsigned int fmt;
>  	int clk_div;
>  	bool i2s_accurate_sck;
> +
> +	int tdm_slots;
> +	int slot_width;
>  };
>  
>  static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
> @@ -213,6 +216,57 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
>  	toggle_clock(dev, playback);
>  }
>  
> +static int davinci_i2s_tdm_word_length(int tdm_slot_width)
> +{
> +	switch (tdm_slot_width) {
> +	case 8:
> +		return DAVINCI_MCBSP_WORD_8;
> +	case 12:
> +		return DAVINCI_MCBSP_WORD_12;
> +	case 16:
> +		return DAVINCI_MCBSP_WORD_16;
> +	case 20:
> +		return DAVINCI_MCBSP_WORD_20;
> +	case 24:
> +		return DAVINCI_MCBSP_WORD_24;
> +	case 32:
> +		return DAVINCI_MCBSP_WORD_32;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai,
> +				    unsigned int tx_mask,
> +				    unsigned int rx_mask,
> +				    int slots, int slot_width)
> +{
> +	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width);

The __func__ can be ommited, it is better to leave it for dynamic
debugging by adding "dyndbg=+pmf" module parameter if needed.

> +
> +	if (slots > 128 || !slots) {
> +		dev_err(dev->dev, "Invalid number of slots\n");
> +		return -EINVAL;
> +	}
> +
> +	if (rx_mask != (1 << slots) - 1) {
> +		dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n",
> +			rx_mask);
> +		return -EINVAL;

This is only a restriction for RX?

> +	}
> +
> +	if (davinci_i2s_tdm_word_length(slot_width) < 0) {
> +		dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width);
> +		return -EINVAL;
> +	}
> +
> +	dev->tdm_slots = slots;
> +	dev->slot_width = slot_width;
> +
> +	return 0;
> +}
> +
>  #define DEFAULT_BITPERSAMPLE	16
>  
>  static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
> @@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>  		DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);
>  
>  	dev->fmt = fmt;
> +
> +	if ((dev->tdm_slots || dev->slot_width) &&
> +	    ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) {
> +		dev_err(dev->dev, "TDM is only supported for BP_FC format\n");
> +		return -EINVAL;

I think this is not a valid statement, Fsync can be generated internally
or coming from external source in TDM mode also.

> +	}
> +
>  	/* set master/slave audio interface */
>  	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
>  	case SND_SOC_DAIFMT_BP_FP:
> @@ -383,7 +444,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  
>  	master = dev->fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK;
>  	fmt = params_format(params);
> -	mcbsp_word_length = asp_word_length[fmt];
> +	if (dev->slot_width)
> +		mcbsp_word_length = davinci_i2s_tdm_word_length(dev->slot_width);
> +	else
> +		mcbsp_word_length = asp_word_length[fmt];
> +
> +	if (mcbsp_word_length < 0)
> +		return mcbsp_word_length;
>  
>  	switch (master) {
>  	case SND_SOC_DAIFMT_BP_FP:
> @@ -483,8 +550,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (master) {
>  	case SND_SOC_DAIFMT_BP_FP:
>  	case SND_SOC_DAIFMT_BP_FC:
> -		rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
> -		xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
> +		if (dev->tdm_slots > 0) {
> +			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(dev->tdm_slots - 1);
> +			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(dev->tdm_slots - 1);
> +		} else {
> +			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
> +			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
> +		}
>  		break;
>  	case SND_SOC_DAIFMT_BC_FC:
>  	case SND_SOC_DAIFMT_BC_FP:
> @@ -609,6 +681,7 @@ static const struct snd_soc_dai_ops davinci_i2s_dai_ops = {
>  	.hw_params	= davinci_i2s_hw_params,
>  	.set_fmt	= davinci_i2s_set_dai_fmt,
>  	.set_clkdiv	= davinci_i2s_dai_set_clkdiv,
> +	.set_tdm_slot   = davinci_i2s_set_tdm_slot,
>  
>  };
>  
> @@ -621,7 +694,7 @@ static struct snd_soc_dai_driver davinci_i2s_dai = {
>  	},
>  	.capture = {
>  		.channels_min = 2,
> -		.channels_max = 2,
> +		.channels_max = 128,
>  		.rates = DAVINCI_I2S_RATES,
>  		.formats = DAVINCI_I2S_FORMATS,
>  	},

-- 
Péter