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

[Jyri Sarha]

On 01/03/2019 14:36, Mark Brown wrote:
> On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
>> On 22/02/2019 23:27, Russell King wrote:
> 
>>> +	/*
>>> +	 * 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.
> 
> So, this is true.  On the other hand like Russell says further down the
> thread it's preserving the existing behaviour so it would be surprising
> if it actually broke anything and it will help systems that explicitly
> set the ratio so I don't think we should let perfect be the enemy of
> good here.
> 

Sure. Still, the requirement for having bclk_ratio of either 32, 48, or
64 is coming from tda998x, so that requirement should be satisfied in
tda998x driver, not in hdmi-codec that is supposed to be generic and
imposes that behaviour to all other user too.

Of course we should not make things overly complicated just because of
such principle. But in this case we are trying to preserve existing
behaviour that has hardly ever worked, and the new behaviour will
potentially cause more trouble. And if we really want to preserve the
existing behaviour there is another way that affects only tda998x driver:

- hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if
  snd_soc_dai_set_bclk_ratio() is not called

- tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the
  implicit bclk_ratio setting the same way as the code above does it

Of course the configurations that we are trying to fix would only work
if the cpu-dai would by some luck (or intentional hack) use bclk_ratio
of 48 for 18- and 20-bit sample formats.

This is the problem with implicit blck_ratio setting at frame clock
slave end. Currently there is only a way to set bclk_ratio from the
machine driver, but there is no way ask what a dai driver would prefer
to use. That is why it is unlikely that 18- or 20-bit sample formats
have worked with any platform with tda998x, or will work in future with
just the implicit bclk_ratio setting.

But, after (hopefully) making my point, if both you and Russell want to
use Russell's original approach, please go ahead. As you said, it is
unlikely that it breaks anything.

> As Russell outlined there's quite a bit of hopeful assumption in how
> ASoC handles the mapping of memory formats onto wire formats which works
> almost all the time but not always and definitely not through robust
> design, that should be a lot easier to address once the component
> conversion has been done as we'll actually have all the links in the
> system directly visible rather than bundled up together and implied as
> they are currently.  Sadly that's a lot of work with not many people
> working on it so progress is super slow.
> 

Yes, there is lot more that could be done there. Ideally there would be
an option to let the dai drivers on the wire to negotiate the i2s format
and related parameters by them selves based on sample width, channels,
and sample rate.... but yes that is not a simple thing to implement.

Best regards,
Jyri

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20190301/d62f785d/attachment.sig>