Re: [PATCH v2 2/2] ASoC: tegra: Unify ASoC machine drivers

[Dmitry Osipenko]

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@xxxxxxxxxx>
>> Co-developed-by: Ion Agorria <ion@xxxxxxxxxxx>
>> Signed-off-by: Ion Agorria <ion@xxxxxxxxxxx>
>> Co-developed-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
>> Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>> ---
>>  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.