All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Osipenko <digetx@gmail.com>
To: Jon Hunter <jonathanh@nvidia.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 22:05:00 +0300	[thread overview]
Message-ID: <91e53907-d87d-aeeb-4644-3926d4311daa@gmail.com> (raw)
In-Reply-To: <32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com>

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@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.

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.

WARNING: multiple messages have this Message-ID
From: Dmitry Osipenko <digetx@gmail.com>
To: Jon Hunter <jonathanh@nvidia.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 22:05:00 +0300	[thread overview]
Message-ID: <91e53907-d87d-aeeb-4644-3926d4311daa@gmail.com> (raw)
In-Reply-To: <32171079-ed4e-1147-2272-5f11bc480c6a@nvidia.com>

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@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.

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.

  reply	other threads:[~2021-05-21 19:05 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
2021-05-21 13:12     ` Jon Hunter
2021-05-21 19:05     ` Dmitry Osipenko [this message]
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=91e53907-d87d-aeeb-4644-3926d4311daa@gmail.com \
    --to=digetx@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=clamor95@gmail.com \
    --cc=ion@agorria.com \
    --cc=jonathanh@nvidia.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.