On 28/02/2019 14:15, Russell King - ARM Linux admin wrote:
Hi all,
While looking at hdmi-codec issues, I spotted this code:
static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) { ... if (params_width(params) > 24) params->msbits = 24;
params->msbits is a parameter that is negotiated and refined by libasound, and is part of the ALSA constraint system. The "Writing an ALSA driver" documentation says about the hw_params callback:
"This is called when the hardware parameter (``hw_params``) is set up by the application, that is, once when the buffer size, the period size, the format, etc. are defined for the pcm substream."
which suggests we should only be reading the parameters, not writing to them.
What's more is that the msbits is a parameter that is refined with userspace, so surely the above should be a declared constraint?
Certainly not a constraint. While HDMI can only pass up to 24-bit per sample audio the most (or at least the two I have worked with) encoders can take 32-bit wide (and probably wider) samples trough i2s. So the system can play 32-bit samples, just the 4 LSB is ignored.
Actually there is very little difference between the case of playing 32-bit stereo audio with matching i2s bclk ratio, and playing 24-bit stereo audio with 64-bit bclk ratio. Especially when the 4 LSB is anyway ignored like in HDMI audio case.
Digging a bit deeper, ASoC passes a private copy of the params to each codec - a copy is made, then fixups for TDM slots are applied, followed by any topology fixups by the DAI link (be_hw_params_fixup) before the codec driver's hw_params() callback is made. Afterwards, ASoC reads back the rate, channels and physical (memory) width and stores them in the codec's DAI structure. The msbits are not read.
hdmi-codec also seems to do nothing with the msbits parameter other than the above code.
So, all in all, it seems that the above code limiting msbits is redundant - nothing will read this modified value. Can we kill it?
It certainly looks that way, so yes. In any case struct snd_soc_dai_driver .playback.sig_bits = 24 should be all that is needed.
Regards, Jyri