Received-SPF: pass (google.com: domain of linux-kernel+bounces-108015-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <00182d1d-ef29-457f-9e3e-6e9b57592118@gmail.com>
Date: Tue, 19 Mar 2024 20:29:38 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin
 during capture streams
Content-Language: en-US
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-14-bastien.curutchet@bootlin.com>
From: =?UTF-8?Q?P=C3=A9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>
In-Reply-To: <20240315112745.63230-14-bastien.curutchet@bootlin.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 15/03/2024 13:27, Bastien Curutchet wrote:
> The McBSP's DX pin that outputs serial data during playback streams can
> be used during capture streams to repeatedly output a chosen pattern.
> For instance, this can be useful to drive an active-low signal during
> captures (by choosing <0> as output pattern).

Are there really any other use of this than to pull down or up the DX
pin (0 or 0xffff)?

If you just use the pin as GPIO then you don't need to change anything
in the driver, The playback would not erach the pin, so no need to block it.

> Enable this behaviour when the device-tree property 'ti,drive-dx' is
> present. DX pin is driven with the provided pattern every time a
> capture stream is launched.

It is an interesting use of the hardware... You are controlling an
external device (light an LED when capture is on)?

> This property is not compatible with classic playback stream so
> davinci_i2s_trigger() returns an error if a playback stream is started
> while 'ti,drive-dx' flag is present.

Propbaly add the .startup() callback and block the playback right there?

> 
> This has been tested on a board designed of a DAVINCI/OMAP-L138 where
> the DX pin is linked to the chip select pin of the converters of the
> capture side.

Isn't the DX will be pulled down as soon as the McBSP is enabled?
Can you just re-configure the PUPD_SEL for the pin group to make the pin
to be pulled the other way?

> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  sound/soc/ti/davinci-i2s.c | 74 ++++++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
> index 13e349e7a6ec..e289a84bdd6a 100644
> --- a/sound/soc/ti/davinci-i2s.c
> +++ b/sound/soc/ti/davinci-i2s.c
> @@ -101,6 +101,9 @@
>  #define DAVINCI_MCBSP_PCR_FSRM		(1 << 10)
>  #define DAVINCI_MCBSP_PCR_FSXM		(1 << 11)
>  
> +#define PLAYBACK_CLOCK			1
> +#define CAPTURE_CLOCK			0
> +
>  enum {
>  	DAVINCI_MCBSP_WORD_8 = 0,
>  	DAVINCI_MCBSP_WORD_12,
> @@ -164,6 +167,8 @@ struct davinci_mcbsp_dev {
>  
>  	bool sync_err;
>  	bool free_run;
> +	bool drive_dx;
> +	u32 dx_val;
>  };
>  
>  static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
> @@ -187,6 +192,19 @@ static void toggle_clock(struct davinci_mcbsp_dev *dev, int playback)
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, dev->pcr);
>  }
>  
> +static int davinci_drive_dx(struct davinci_mcbsp_dev *dev)
> +{
> +	unsigned int spcr;
> +
> +	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val);
> +
> +	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
> +	spcr |= DAVINCI_MCBSP_SPCR_XRST;
> +	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);


> +
> +	return 0;
> +}
> +
>  static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
>  		struct snd_pcm_substream *substream)
>  {
> @@ -194,6 +212,9 @@ static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
>  	u32 spcr;
>  	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
>  
> +	if (!playback && dev->drive_dx)
> +		davinci_drive_dx(dev);
> +
>  	/* Enable transmitter or receiver */
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  	spcr |= mask;

if (dev->drive_dx) {
	spcr |= DAVINCI_MCBSP_SPCR_XRST;
	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val);
}

and no need for the davinci_drive_dx() function, plus it makes it
symmetric of what you do on stop()

> @@ -212,9 +233,17 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
>  	/* Reset transmitter/receiver and sample rate/frame sync generators */
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  	spcr &= ~(DAVINCI_MCBSP_SPCR_GRST | DAVINCI_MCBSP_SPCR_FRST);
> -	spcr &= playback ? ~DAVINCI_MCBSP_SPCR_XRST : ~DAVINCI_MCBSP_SPCR_RRST;
> -	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> -	toggle_clock(dev, playback);
> +
> +	if (!playback) {
> +		spcr &= ~DAVINCI_MCBSP_SPCR_RRST;
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> +		toggle_clock(dev, CAPTURE_CLOCK);
> +	}
> +	if (playback || dev->drive_dx) {
> +		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
> +	}
>  }
>  
>  static int davinci_i2s_tdm_word_length(int tdm_slot_width)
> @@ -408,6 +437,10 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>  	}
>  	if (inv_fs == true)
>  		pcr ^= (DAVINCI_MCBSP_PCR_FSXP | DAVINCI_MCBSP_PCR_FSRP);
> +
> +	if (dev->drive_dx)
> +		pcr |= DAVINCI_MCBSP_PCR_CLKRP;
> +
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr);
>  	dev->pcr = pcr;
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, pcr);
> @@ -562,6 +595,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1);
>  	}
>  
> +	if (dev->drive_dx)
> +		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(2);
> +
>  	if (params_channels(params) == 2) {
>  		element_cnt = 2;
>  		if (double_fmt[fmt] && dev->enable_channel_combine) {
> @@ -611,9 +647,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  	xcr |= DAVINCI_MCBSP_XCR_XWDLEN1(mcbsp_word_length) |
>  		DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length);
>  
> -	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK || dev->drive_dx)
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_XCR_REG, xcr);
> -	else
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_RCR_REG, rcr);
>  
>  	pr_debug("%s - %d  srgr=%X\n", __func__, __LINE__, srgr);
> @@ -628,16 +664,21 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai);
>  	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
>  	u32 spcr;
> -	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
>  
>  	davinci_mcbsp_stop(dev, playback);
>  
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
> -	if (spcr & mask) {
> +	if (spcr & DAVINCI_MCBSP_SPCR_XRST) {
>  		/* start off disabled */
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
> -					spcr & ~mask);
> -		toggle_clock(dev, playback);
> +					spcr & ~DAVINCI_MCBSP_SPCR_XRST);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
> +	}
> +	if (spcr & DAVINCI_MCBSP_SPCR_RRST) {
> +		/* start off disabled */
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
> +					spcr & ~DAVINCI_MCBSP_SPCR_RRST);
> +		toggle_clock(dev, CAPTURE_CLOCK);
>  	}
>  	if (dev->pcr & (DAVINCI_MCBSP_PCR_FSXM | DAVINCI_MCBSP_PCR_FSRM |
>  			DAVINCI_MCBSP_PCR_CLKXM | DAVINCI_MCBSP_PCR_CLKRM)) {
> @@ -646,7 +687,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
>  	}
>  
> -	if (playback) {
> +	if (playback || dev->drive_dx) {
>  		/* Enable the transmitter */
>  		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  		spcr |= DAVINCI_MCBSP_SPCR_XRST;
> @@ -659,7 +700,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> -		toggle_clock(dev, playback);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
>  	}
>  
>  	return 0;
> @@ -672,6 +713,11 @@ static int davinci_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  	int ret = 0;
>  	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
>  
> +	if (playback && dev->drive_dx) {
> +		dev_err(dev->dev, "Playback is not allowed when drive-cs flag is set\n");
> +		return -EINVAL;
> +	}
> +
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
> @@ -779,6 +825,12 @@ static int davinci_i2s_probe(struct platform_device *pdev)
>  
>  	dev->free_run = !of_property_read_bool(pdev->dev.of_node, "ti,disable-free-run");
>  	dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err");
> +	dev->drive_dx = false;

no need to initialize it to 0, dev is allocated with devm_kzalloc()

> +	ret = of_property_read_u32(pdev->dev.of_node, "ti,drive-dx", &dev->dx_val);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +	if (!ret)
> +		dev->drive_dx = true;

if (!ret)
	dev->drive_dx = true;
else if (ret != -EINVAL)
	return ret;

>  
>  	/* setup DMA, first TX, then RX */
>  	dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];

-- 
Péter