On 25/02/2019 16:03, Russell King - ARM Linux admin wrote:
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
Some HDMI codecs need to know the relationship between the I2S bit clock and the I2S word clock in order to correctly generate the CTS value for audio clock recovery on the sink.
Add support for this, but there are currently no callers of snd_soc_dai_set_bclk_ratio(), we provide a default implementation that uses the sample width to derive the ratio from the 8-bit aligned sample size. This reflects the derivation that is in TDA998x, which we are going to convert to use this new support.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1;
- unsigned int bclk_ratio;
};
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..d71a7e5a2231 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_daifmt fmt; struct hdmi_codec_params hp = { .iec = { .status = { 0 },
@@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_rate = params_rate(params); hp.channels = params_channels(params);
- fmt = hcp->daifmt[dai->id];
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
As mentioned in the commit message, this is what TDA998x does today, and I have to assume that the contributor tested this, who seems to be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata").
Yes, my original implementation was a bit optimistic. I only had limited information about the chip and a somewhat working undocumented hackish driver implementation. By now it's clear that rejecting anything but 16-, 24-, or 32-bit sample widths (32, 48, or 64 bclk ratios) would have been right way to go.
If we don't do it this way, then converting TDA998x to use this defaulting would change the already established behaviour. As there are no users of this by hdmi-codec implementations, and as there are no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain the current behaviour in TDA998x is to either place a default in hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have that encode the above.
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
This way my bad choices would not spread to all hdmi-codec users.
Then again, I don't think anyone is using 18- or 20-bit samples with tda998x. They most likely do not even work. So simply refusing the 36 and 40 blck_ratios would probably be just fine too.
I would much rather every user of hdmi-codec gained support for snd_soc_dai_set_bclk_ratio() rather than relying on that default, but that is beyond what I can do - I don't have the knowledge of which sound setups would need it, and I don't have any platforms that are configured to use I2S with TDA998x.
The little that I know how ASoC is generally used, it is no wonder that so few drivers have implemented it. In 99% of the cases that I have encountered the bclk_ratio is sample_width*channels. I cases where something else is needed the blck_ratio is not the only missing piece.
The Dove Cubox has spdif and i2s but is setup to use spdif (since that is the most flexible, supporting compressed audio.) I've a large pile of unsubmittable patches there which makes kirkwood use DPCM, as they would end up breaking all the existing users, and from what I can see there's no way around that. That makes submitting a patch to add snd_soc_dai_set_bclk_ratio() very difficult. That said, the above defaults are not correct for kirkwood-i2s - that always uses bclk running at 64fs, as per the TDA998x code prior to your patch I mention above.
ARM Juno has two TDA998x, but the DT description lacks any audio description, so it's not clear whether these are even wired.
The ARM MPS platform has a TDA998x connected to a HDLCD, but the HDLCD does not have an interrupt - and the DRM driver requires an interrupt. The conclusion that was come to there is basically "don't even bother". Also unknown whether audio is wired up.