Re: [PATCH 13/14] ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102

[Hans de Goede]

Hi,

Thank you for the review.

On 12/29/20 2:58 PM, Charles Keepax wrote:
> On Sun, Dec 27, 2020 at 10:12:31PM +0100, Hans de Goede wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>>
>> Add a new ASoc Machine driver for Intel Baytrail platforms with a
>> Wolfson Microelectronics WM5102 codec.
>>
>> This is based on a past contributions [1] from Paulo Sergio Travaglia
>> <pstglia@xxxxxxxxx> based on the Levono kernel [2] combined with
>> insights in things like the speaker GPIO from the android-x86 android
>> port for the Lenovo Yoga Tablet 2 1051F/L [3].
>>
>> [1] https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e05.47e9@xxxxxxxxxxxxx/
>> [2] https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.1/sound/soc/intel/board/byt_bl_wm5102.c
>> [3] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
>>
>> The original machine driver from the Android ports was a crude modified
>> copy of bytcr_rt5640.c adjusted to work with the WM5102 codec.
>> This version has been extensively reworked to:
>>
>> 1. Remove all rt5640 related quirk handling. to the best of my knowledge
>> this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13
>> inch models) which all use the same setup. So there is no need to deal
>> with all the variations with which we need to deal on rt5640 boards.
>>
>> 2. Rework clock handling, properly turn off the FLL and the platform-clock
>> when they are no longer necessary and don't reconfigure the FLL
>> unnecessarily when it is already running. This fixes a number of:
>> "Timed out waiting for lock" warnings being logged.
>>
>> 3. Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY
>>
>> This only adds the machine driver and ACPI hooks, the BYT-CR detection
>> quirk which these devices need will be added in a separate patch.
>>
>> Co-authored-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
> 
> Just a couple really minor comments.
> 
>> +static int byt_wm5102_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate)
>> +{
>> +	struct snd_soc_component *codec_component = codec_dai->component;
>> +	int sr_mult = ((rate % 4000) == 0) ?
>> +		(WM5102_MAX_SYSCLK_4K / rate) :
>> +		(WM5102_MAX_SYSCLK_11025 / rate);
>> +	int ret;
>> +
>> +	/* Reset FLL1 */
>> +	snd_soc_dai_set_pll(codec_dai, WM5102_FLL1_REFCLK, ARIZONA_FLL_SRC_NONE, 0, 0);
>> +	snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0);
>> +
>> +	/* Configure the FLL1 PLL before selecting it */
>> +	ret = snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_CLK_SRC_MCLK1,
>> +				  MCLK_FREQ, rate * sr_mult);
>> +	if (ret) {
>> +		dev_err(codec_component->dev, "Error setting PLL: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_SYSCLK,
>> +					   ARIZONA_CLK_SRC_FLL1, rate * sr_mult,
>> +					   SND_SOC_CLOCK_IN);
>> +	if (ret) {
>> +		dev_err(codec_component->dev, "Error setting ASYNCCLK: %d\n", ret);
> 
> Error message should say SYSCLK not ASYNCCLK.

Fixed for v2.

> 
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK, 0,
>> +					   rate * sr_mult, SND_SOC_CLOCK_OUT);
>> +	if (ret) {
>> +		dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret);
>> +		return ret;
>> +	}
> 
> OPCLK is a clock that can be outputted on the CODECs GPIOs. Is
> that being used to clock some external component? If so it should
> be added to the DAPM graph, if not you might as well remove this
> call.

I copy pasted this from the work done for Android X86 to get sound to
work on the Lenovo Tablet 2 series:
https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel

I believe when you say it is unnecessary, so I will remove it for v2
(and test without this present to make sure it is really unnecessary).

> 
>> +
>> +	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK,
>> +				     rate * 512, SND_SOC_CLOCK_IN);
>> +	if (ret) {
>> +		dev_err(codec_component->dev, "Error setting clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
> 
> The rate you set here doesn't actually matter, on wm5102 this
> just links the DAI to a specific clock domain and as they all
> default to SYSCLK you can omit this call if you want. Although no
> harm is caused by leaving it in.

I'm going to leave this in as I prefer to be explicit about things like
this, rather then relying on defaults.

Regards,

Hans