Re: [PATCH 03/13] drm/i2c: tda998x: improve programming of audio divisor - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Sven Van Asbroeck <thesven73@gmail.com>
Cc: David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Mark Brown <broonie@kernel.org>, Jyri Sarha <jsarha@ti.com>
Subject: Re: [PATCH 03/13] drm/i2c: tda998x: improve programming of audio divisor
Date: Wed, 12 Jun 2019 17:26:00 +0100	[thread overview]
Message-ID: <20190612162600.q4otimitocjeg6ln@shell.armlinux.org.uk> (raw)
In-Reply-To: <CAGngYiV5FCU8RPfn2TFEJSWSWvD9VmpKWcPb_0TBpWc6nwho+g@mail.gmail.com>

On Wed, Jun 12, 2019 at 11:25:59AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:02 AM Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Improve the selection of the audio clock divisor so that more modes
> > and sample rates work.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> 
> +static u8 tda998x_get_adiv(struct tda998x_priv *priv, unsigned int fs)
> +{
> +       unsigned long min_audio_clk = fs * 128;
> +       unsigned long ser_clk = priv->tmds_clock * 1000;
> +       u8 adiv;
> +
> +       for (adiv = AUDIO_DIV_SERCLK_32; adiv != AUDIO_DIV_SERCLK_1; adiv--)
> +               if (ser_clk > min_audio_clk << adiv)
> +                       break;
> +
> +       dev_dbg(&priv->hdmi->dev,
> +               "ser_clk=%luHz fs=%uHz min_aclk=%luHz adiv=%d\n",
> +               ser_clk, fs, min_audio_clk, adiv);
> +
> +       return adiv;
> 
> Doesn't this function need an error return in case min_audio_clk > ser_clk ?
> Or is that a situation that can never arise?

It's possible it could arise.  For example, if we have a 192kHz sample
rate, and a tmds clock slower than 24.576MHz.

In such a case, we will select AUDIO_DIV_SERCLK_1 as the divisor, which
is a legal value.  The result _may_ be audio not working (which is what
already happens today when we get this setting wrong.)

If we were to return an error, there's no way to handle that error, so
what's the point of returning an error?

The results of this function match what the Philips firmware uses for
this register for all standard TMDS clocks and sample rates, so it's
not a problem that I'm particularly concerned about at this point.
The only way around this would be to increase the TMDS clock, which
means using pixel doubling tricks, and therefore a mode set.  I don't
think any HDMI driver has the capability to deal with that yet.

-- 
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel