Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
From: Russell King - ARM Linux admin
Date: Fri Feb 22 2019 - 16:36:24 EST
On Fri, Feb 22, 2019 at 04:18:33PM -0500, Sven Van Asbroeck wrote:
> Russell, thank you so much for your patience, help and explanation, I really
> appreciate it !
>
> Yes, that would keep the current users in business without them having to
> change anything.
>
> Of course, poor souls like myself would have to patch, say, simple-card so we
> could provide a bclk ratio in the devicetree. Which would then propagate down
> to the tda998x via hdmi-codec. Which is fine by me.
It probably would be better to try and find some generic way to deal
with this.
After all, the I2S source probably knows which ratios it supports.
Given that many sinks support a limited set of values as well, if
ASoC core knew the supported set at each end of an I2S DAI format
link, it could probably select a working bclk ratio automatically.
> Combining your two previous suggestions, I get the following. Now sample_width
> can be removed from tda998x_audio_params, as it's no longer used.
>
> Would this be a good start?
There's actually two threads of conversation going, and I recently had
a reply from the maintainer of hdmi-codec suggesting a way forward - so
I've coded that up as the three RFC patches you should have just
received.
That should allow you to merely add a snd_soc_dai_set_bclk_ratio() call
to set the bclk ratio while avoiding any breakage for existing users.
It does still contain my "FIXME" comment, so it isn't the final
solution yet. One of the down-sides to the hdmi-codec shim is that it
doesn't know which DAI formats nor which bclk ratios the hdmi-codec
it's attached to supports, which makes validation against the codec
capabilities rather awkward.
Sorry to have cut across your patch below.
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..a306994ecc79 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -893,19 +893,25 @@ tda998x_configure_audio(struct tda998x_priv *priv,
> reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
> clksel_aip = AIP_CLKSEL_AIP_I2S;
> clksel_fs = AIP_CLKSEL_FS_ACLK;
> - switch (params->sample_width) {
> + switch (params->bclk_ratio) {
> case 16:
> + cts_n = CTS_N_M(3) | CTS_N_K(0);
> + break;
> + case 32:
> cts_n = CTS_N_M(3) | CTS_N_K(1);
> break;
> - case 18:
> - case 20:
> - case 24:
> + case 48:
> cts_n = CTS_N_M(3) | CTS_N_K(2);
> break;
> - default:
> - case 32:
> + case 64:
> cts_n = CTS_N_M(3) | CTS_N_K(3);
> break;
> + case 128:
> + cts_n = CTS_N_M(0) | CTS_N_K(0);
> + break;
> + default:
> + dev_err(&priv->hdmi->dev, "unsupported I2S bclk ratio\n");
> + return -EINVAL;
> }
> break;
>
> @@ -982,7 +988,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
> struct tda998x_priv *priv = dev_get_drvdata(dev);
> int i, ret;
> struct tda998x_audio_params audio = {
> - .sample_width = params->sample_width,
> + .bclk_ratio = daifmt->bclk_ratio,
> .sample_rate = params->sample_rate,
> .cea = params->cea,
> };
> @@ -1004,6 +1010,23 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
> if (priv->audio_port[i].format == AFMT_I2S)
> audio.config = priv->audio_port[i].config;
> audio.format = AFMT_I2S;
> + if (!audio.bclk_ratio) {
> + /* compatibility */
> + switch (params->sample_width) {
> + case 16:
> + audio.bclk_ratio = 32;
> + break;
> + case 18:
> + case 20:
> + case 24:
> + audio.bclk_ratio = 48;
> + break;
> + default:
> + case 32:
> + audio.bclk_ratio = 64;
> + break;
> + }
> + }
> break;
> case HDMI_SPDIF:
> for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 3cb25ccbe5e6..039e1d9af2e0 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -14,7 +14,7 @@ enum {
> struct tda998x_audio_params {
> u8 config;
> u8 format;
> - unsigned sample_width;
> + u8 bclk_ratio;
> unsigned sample_rate;
> struct hdmi_audio_infoframe cea;
> u8 status[5];
> 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 d00734d31e04..d82f26854c90 100644
> --- a/sound/soc/codecs/hdmi-codec.c
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> &hcp->daifmt[dai->id], &hp);
> }
>
> +static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai,
> + unsigned int ratio)
> +{
> + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> +
> + /* FIXME: some validation here would be good? */
> + hcp->daifmt[dai->id].bclk_ratio = ratio;
> +
> + return 0;
> +}
> +
> static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
> unsigned int fmt)
> {
> @@ -593,7 +604,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
> }
> }
>
> - hcp->daifmt[dai->id] = cf;
> + hcp->daifmt[dai->id].fmt = cf.fmt;
> + hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv;
> + hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv;
> + hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master;
> + hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
>
> return ret;
> }
> @@ -615,6 +630,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = {
> .startup = hdmi_codec_startup,
> .shutdown = hdmi_codec_shutdown,
> .hw_params = hdmi_codec_hw_params,
> + .set_bclk_ratio = hdmi_codec_set_bclk_ratio,
> .set_fmt = hdmi_codec_set_fmt,
> .digital_mute = hdmi_codec_digital_mute,
> };
> --
> 2.17.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up