All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jon Hunter <jonathanh@nvidia.com>
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>
Subject: Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers
Date: Fri, 21 May 2021 14:12:23 +0100	[thread overview]
Message-ID: <32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com> (raw)
In-Reply-To: <20210520175054.28308-3-digetx@gmail.com>


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

WARNING: multiple messages have this Message-ID
From: Jon Hunter <jonathanh@nvidia.com>
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: linux-tegra@vger.kernel.org, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers
Date: Fri, 21 May 2021 14:12:23 +0100	[thread overview]
Message-ID: <32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com> (raw)
In-Reply-To: <20210520175054.28308-3-digetx@gmail.com>


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

  parent reply	other threads:[~2021-05-21 13:12 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20 17:50 [PATCH v2 0/2] Unify NVIDIA Tegra " Dmitry Osipenko
2021-05-20 17:50 ` Dmitry Osipenko
2021-05-20 17:50 ` [PATCH v2 1/2] ASoC: tegra: Set driver_name=tegra for all " Dmitry Osipenko
2021-05-20 17:50   ` Dmitry Osipenko
2021-05-20 18:12   ` Dmitry Osipenko
2021-05-20 18:12     ` Dmitry Osipenko
2021-05-20 17:50 ` [PATCH v2 2/2] ASoC: tegra: Unify ASoC " Dmitry Osipenko
2021-05-20 17:50   ` Dmitry Osipenko
2021-05-20 19:02   ` Jaroslav Kysela
2021-05-20 19:02     ` Jaroslav Kysela
2021-05-20 19:08     ` Mark Brown
2021-05-20 19:08       ` Mark Brown
2021-05-21  8:54       ` Jaroslav Kysela
2021-05-21  8:54         ` Jaroslav Kysela
2021-05-21 18:43     ` Dmitry Osipenko
2021-05-21 18:43       ` Dmitry Osipenko
2021-05-21 13:12   ` Jon Hunter [this message]
2021-05-21 13:12     ` Jon Hunter
2021-05-21 19:05     ` Dmitry Osipenko
2021-05-21 19:05       ` Dmitry Osipenko
2021-05-24 12:22       ` Jon Hunter
2021-05-24 12:22         ` Jon Hunter
2021-05-24 13:40         ` Dmitry Osipenko
2021-05-24 13:40           ` Dmitry Osipenko
2021-05-24 18:50           ` Jon Hunter
2021-05-24 18:50             ` Jon Hunter
2021-05-24 21:02             ` Dmitry Osipenko
2021-05-24 21:02               ` Dmitry Osipenko
2021-05-25  6:51               ` Jon Hunter
2021-05-25  6:51                 ` Jon Hunter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com \
    --to=jonathanh@nvidia.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=clamor95@gmail.com \
    --cc=digetx@gmail.com \
    --cc=ion@agorria.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=thierry.reding@gmail.com \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.