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@xxxxxxxxxxxxxxx>
>>> ---
>>> 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.
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
