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