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
