Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers
To:     Dmitry Osipenko <digetx@gmail.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Mark Brown <broonie@kernel.org>, Takashi Iwai <tiwai@suse.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Ion Agorria <ion@agorria.com>,
        Svyatoslav Ryhel <clamor95@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>
CC:     <alsa-devel@alsa-project.org>, <linux-tegra@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20210520175054.28308-1-digetx@gmail.com>
 <20210520175054.28308-3-digetx@gmail.com>
From:   Jon Hunter <jonathanh@nvidia.com>
Message-ID: <32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com>
Date:   Fri, 21 May 2021 14:12:23 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210520175054.28308-3-digetx@gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2021 13:12:28.7613
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: d7cc4c8a-f3a0-47f8-14d1-08d91c5a15e1
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT053.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4083
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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@nvidia.com>
> Co-developed-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.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.


> +	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?

> +
> +	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?

Cheers
Jon

-- 
nvpublic