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

[Xingyu Wu]

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.

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.

> 
> >
> >>
> >>> +
> >>> +	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()?

Best regards,
Xingyu Wu