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

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 4/16/24 02:23, Xingyu Wu wrote:
> On 02/04/2024 21:57, Pierre-Louis Bossart wrote:
>>
>>
>>>>
>>>>> +#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.
> 
> Yes, this position will be used later in the cdns_i2s_pcm_pointer().
> But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which
> would be frequently called several times each period. The period_pos
> is used to determine whether there is enough a period_size to call
> snd_pcm_period_elapsed().
> 
>>
>> 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.
>>
> 
> a) It must enable clocks first to read and write registers when I2S probe.
> Then it is done to probe, the clocks are still enabled and the state of pm
> is suspend. So it need to be disabled to match the state and will resume
> and be enabled by ALSA.

I think you are missing a pm_runtime_set_active() to reconcile the pm
state with the hardware state. The premise of pm_runtime is that on
probe your device is active and later on it will suspend. Having
pm_runtime_enabled with a suspended device without the framework
involved to trigger the transition to suspend is asking for trouble.

> b) Because CONFIG_PM would be disabled and pm_runtime_enabled()
> return false , then it is no need to disable clock and I2S still can work.

Again you are trying to make things more complicated than they need to
be. Don't try to actively manage and query states, let the framework do
it for you.

Try to probe and bring the device to an active state. Then use
pm_runtime_mark_last_busy(), use pm_runtime_enable and let autosuspend
do the work for you. If pm_runtime is not enabled the suspend will not
happen.

Also keep in mind that pm_runtime_enabled() will return false if the
user mucks with the power state in sysfs, it's not only a case of
CONFIG_PM being selected or not.
> 
>>
>>>
>>>>
>>>>> +
>>>>> +	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.
> 
> Should I use the pm_runtime_enabled() before the pm_runtime_disable()?

It doesn't matter, the problem is the second part where you try to check
the status of pm_runtime *after* disabling it.