| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <91e53907-d87d-aeeb-4644-3926d4311daa@gmail.com>
Date: Fri, 21 May 2021 22:05:00 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Jon Hunter <jonathanh@...dia.com>,
Thierry Reding <thierry.reding@...il.com>,
Mark Brown <broonie@...nel.org>, Takashi Iwai <tiwai@...e.com>,
Jaroslav Kysela <perex@...ex.cz>,
Ion Agorria <ion@...rria.com>,
Svyatoslav Ryhel <clamor95@...il.com>,
Liam Girdwood <lgirdwood@...il.com>
Cc: alsa-devel@...a-project.org, linux-tegra@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers
21.05.2021 16:12, Jon Hunter пишет:
>
> On 20/05/2021 18:50, Dmitry Osipenko wrote:
>> Squash all machine drivers into a single-universal one. This reduces
>> code duplication, eases addition of a new drivers and upgrades older
>> code to a modern Linux kernel APIs.
>>
>> Suggested-by: Jonathan Hunter <jonathanh@...dia.com>
>> Co-developed-by: Ion Agorria <ion@...rria.com>
>> Signed-off-by: Ion Agorria <ion@...rria.com>
>> Co-developed-by: Svyatoslav Ryhel <clamor95@...il.com>
>> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>> ---
>> sound/soc/tegra/Kconfig | 12 +
>> sound/soc/tegra/Makefile | 18 +-
>> sound/soc/tegra/tegra20_ac97.c | 1 -
>> sound/soc/tegra/tegra_alc5632.c | 260 ----------
>> sound/soc/tegra/tegra_asoc_machine.c | 732 +++++++++++++++++++++++++++
>> sound/soc/tegra/tegra_asoc_machine.h | 45 ++
>> sound/soc/tegra/tegra_max98090.c | 277 ----------
>> sound/soc/tegra/tegra_rt5640.c | 223 --------
>> sound/soc/tegra/tegra_rt5677.c | 325 ------------
>> sound/soc/tegra/tegra_sgtl5000.c | 212 --------
>> sound/soc/tegra/tegra_wm8753.c | 186 -------
>> sound/soc/tegra/tegra_wm8903.c | 358 +++----------
>> sound/soc/tegra/tegra_wm9712.c | 167 ------
>> sound/soc/tegra/trimslice.c | 173 -------
>> 14 files changed, 862 insertions(+), 2127 deletions(-)
>> delete mode 100644 sound/soc/tegra/tegra_alc5632.c
>> create mode 100644 sound/soc/tegra/tegra_asoc_machine.c
>> create mode 100644 sound/soc/tegra/tegra_asoc_machine.h
>> delete mode 100644 sound/soc/tegra/tegra_max98090.c
>> delete mode 100644 sound/soc/tegra/tegra_rt5640.c
>> delete mode 100644 sound/soc/tegra/tegra_rt5677.c
>> delete mode 100644 sound/soc/tegra/tegra_sgtl5000.c
>> delete mode 100644 sound/soc/tegra/tegra_wm8753.c
>> delete mode 100644 sound/soc/tegra/tegra_wm9712.c
>> delete mode 100644 sound/soc/tegra/trimslice.c
>
> ...
>
>> +static unsigned int tegra_max98090_mclk_rate(unsigned int srate)
>> +{
>
> Minor comment, but I wonder if there is a better name for the above
> function? This function is using a fixed rate as opposed to scaling it
> with sample rate which can be common and not really specific to the
> max98090 codec.
I'll rename it in v3, thank you for suggestion.
>> + unsigned int mclk;
>> +
>> + switch (srate) {
>> + case 8000:
>> + case 16000:
>> + case 24000:
>> + case 32000:
>> + case 48000:
>> + case 64000:
>> + case 96000:
>> + mclk = 12288000;
>> + break;
>> + case 11025:
>> + case 22050:
>> + case 44100:
>> + case 88200:
>> + mclk = 11289600;
>> + break;
>> + default:
>> + mclk = 12000000;
>> + break;
>> + }
>> +
>> + return mclk;
>> +}
>> +
>> +unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
>> +{
>> + unsigned int mclk;
>> +
>> + switch (srate) {
>> + case 64000:
>> + case 88200:
>> + case 96000:
>> + mclk = 128 * srate;
>> + break;
>> + default:
>> + mclk = 256 * srate;
>> + break;
>> + }
>> + /* FIXME: Codec only requires >= 3MHz if OSR==0 */
>> + while (mclk < 6000000)
>> + mclk *= 2;
>
> So this appears to be specific to the wm8903 codec or at least this is
> where it came from. And given that the switch statement is not complete
> in terms of the sample rates (ie. only has a subset), I am wondering if
> set should keep this specific to the wm8903 codec?
The RT5631 codec of Asus Transformers will re-use this function.
IIUC, the default switch-case works properly for all rates below 64KHz,
at least I haven't had any problems with it. Could you please clarify
why you are saying that the switch statement appears to be incomplete?
>> +
>> + return mclk;
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_asoc_machine_mclk_rate);> +
>> +static int tegra_machine_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params)
>> +{
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>> + struct snd_soc_card *card = rtd->card;
>> + struct tegra_machine *machine = snd_soc_card_get_drvdata(card);
>> + unsigned int srate = params_rate(params);
>> + unsigned int mclk = machine->asoc->mclk_rate(srate);
>> + const unsigned int clk_id = 0;
>> + int err;
>> +
>> + err = tegra_asoc_utils_set_rate(&machine->util_data, srate, mclk);
>> + if (err < 0) {
>> + dev_err(card->dev, "Can't configure clocks: %d\n", err);
>> + return err;
>> + }
>> +
>> + err = snd_soc_dai_set_sysclk(codec_dai, clk_id, mclk, SND_SOC_CLOCK_IN);
>
> Looks like clk_id is always 0. Most likely all the clock ids passed are
> 0 by default but I wonder if we should not assume this in case something
> changes in the future?
Initially I had the same thought and even made the clk_id customizable,
but then decided that for now it will be cleaner to hardcode ID to 0
since it will be very easy to customize the ID if will become necessary.
None of the currently supported devices use a different ID. I see now
that the older Galaxy Tab 10 may need to use ID=1, so perhaps indeed it
won't hurt to make it customizable already. I'll reconsider it for v3.
Thank you for the review.
Powered by blists - more mailing lists