On Mon, Apr 01, 2024 at 12:53:18PM +0200, Oswald Buddenhagen wrote:
On Mon, Apr 01, 2024 at 11:37:27AM +0100, Russell King (Oracle) wrote:
runtime->hw.fifo_size is only measured in _frames_ if SNDRV_PCM_INFO_FIFO_IN_FRAMES is set.
yes, which is exactly why the other hunk in the patch sets it.
So now I have to ask what caused you to generate this patch. I don't think you've actually run this driver, so presumably it's by [] code inspection...
yes, it was a random find while trying to make sense of this parameter.
The driver worked when I wrote it. The fifo_size contents was correct when I wrote it. The choice for using bytes here and not setting SNDRV_PCM_INFO_FIFO_IN_FRAMES means that ALSA correctly takes the real FIFO size (which is in bytes) and correctly translates that itself into frames.
I fail to see how this is any better - in fact, I think it's a lot worse because, as you've pointed out, it doesn't deal with stereo. In fact, it only supports 16-bit mono, whereas the driver supports lots more than that.
I think where the confusion comes from is that fifo_depth is the depth of the hardware FIFO in units of 16-bit quantities, which is why we multiply by two to get to bytes. 16-bit quantities does not necessarily equate to ALSA frames - it can be in specific cases but not always.
As far as I'm concerned, the code is correct as it stands and your patch will introduce regressions.