On Fri, Dec 21, 2018 at 03:05:51PM +0200, Dimitris Papavasiliou wrote:
Now, I apologize in advance, but I'll ask you if you can comment on the following final issue I have with this driver, which I was determined to ignore, but can't: I've already mentioned that I'm using a card made for the Raspberry Pi, and I can't really apply my patch there as-is, because they seem to have changed the pcm512x CODEC driver, without pushing the changes upstream. You can see the patch in question here:
https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e5...
I didn't upstream the patch as I wasn't really satisfied with it and also think it's quite hackish.
Back then when the Hifiberry folks submitted their machine driver to the RPi kernel they sneaked in a change to the pcm512x which forced S24 data to use 64fs clocking https://github.com/raspberrypi/linux/pull/1152
This change had the nasty side effect that using pcm512x eg in slave mode with the simple card driver broke as the setup on both sides no longer matched. bcm2835 by default uses 32/48/64fs for 16/24/32 bit data - like most drivers do (also upstream pcm512x driver). pcm512x with the change used 32/64/64fs - something that's not possible to express in simple-card's binding.
So that change had to go and after dropping it we realized some control over fs was needed if the machine driver wanted to support S24_LE (just dropping S24_LE support and relying on alsalib to do automatic 24->32bit conversion would have been another option).
Doing that via the set_bclk_ratio interface would probably have been the cleaner approach, OTOH set_tdm_slot had the nice feature that it already had simple card DT bindings - so I (ab)used that.
I don't have a Hifiberry DAC+ card (or other cards with on-board oscillators plus pcm512x in master mode) so I haven't looked into better solutions. Main focus was to have some workaround that didn't cause collateral damage but allowed the card to keep working.
I wouldn't mind at all if you do a proper implementation and submit it upstream so the downstream patch can be dropped.
so long,
Hias
(My patch, by the way, does run without the above change if you use the card without the external crystals, so I was able to test it in the form I submitted it in.)
I can apply the patch via merge, so bothering with this is not at all necessary, but if this change is useful, perhaps it's worth submitting another patch for it. I doubt this though, as it looks a bit hackish to me.
To provide you with some context: The card uses a 24.576MHz crystal for 48kHz-derived sampling rates, and it supports the S24_LE format, which leads to non-integer LRCK dividers, and somewhat faster-than-normal playback. A solution to this, would be to play 24bit samples as 32-bit with the MSB zeroed out, as the chip supports this. As far as I understand it, the patch implements this, by sending stereo streams as TDM data with two slots, one for each channel, wide enough for the physical size of the sample, which "happens" to be 32bits. On the machine driver size the following is done:
width = snd_pcm_format_physical_width(params_format(params)); ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x03, 0x03, channels, width); ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0x03, 0x03, channels, width);
This works, but I'm not sure TDM is meant for things like that. Anyway, if you think it's worth disucssing this, I can start a new thread if you prefer. If there's a straightforward way to fix this at the machine driver level, that'd be even better, as it would allow me to get rid of the divergence between the Pi and mainline kernel, without bothering the latter. If none of the above is the case, I can happily just merge the patch as-is. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel