Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha <jsarha@ti.com>]

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.
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki