Received-SPF: pass (google.com: domain of linux-kernel+bounces-129736-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ef510647-c409-4da6-9cd4-ff4e54cbee74@linux.intel.com>
Date: Tue, 2 Apr 2024 08:57:14 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: =?UTF-8?B?UmU6IOWbnuWkjTogW1BBVENIIHYyIDIvMl0gQVNvQzogY2RuczogQWRk?=
 =?UTF-8?Q?_drivers_of_Cadence_Multi-Channel_I2S_Controller?=
To: Xingyu Wu <xingyu.wu@starfivetech.com>,
 Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 Claudiu Beznea <Claudiu.Beznea@microchip.com>,
 Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor.dooley@microchip.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
 "linux-sound@vger.kernel.org" <linux-sound@vger.kernel.org>
References: <20240320090239.168743-1-xingyu.wu@starfivetech.com>
 <20240320090239.168743-3-xingyu.wu@starfivetech.com>
 <1d0399d2-684f-490e-8711-f636e987a0b8@linux.intel.com>
 <NTZPR01MB0956BFADB4B3DA507D938F669F35A@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn>
Content-Language: en-US
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <NTZPR01MB0956BFADB4B3DA507D938F669F35A@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


>>
>>> +#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.

That didn't answer to my question, the position is usually between
0..buffer_size.1.

Doing increments on a modulo value then comparisons as done below seems
rather questionable.
	
>>> +
>>> +		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;
>>> +}

>>> +	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.

That is very odd on two counts
a) if you haven't enabled the clocks, why do you need to disbale them?
b) if you do a pm_runtime_enable(), then the branch if
(pm_runtime_enabled) is always true.


> 
>>
>>> +
>>> +	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.

if you are disabling pm_runtime, then the pm_runtime state becames invalid.
When pm_runtime_disable() is added in remove operations, it's mainly to
prevent the device from suspending.