lkml.org 
[lkml]   [2021]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 13/14] ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102
From
Date
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@linux.intel.com>
>>
>> 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@gmail.com> 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@mx.google.com/
>> [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@linux.intel.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>
> 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

\
 
 \ /
  Last update: 2021-01-16 18:10    [W:0.111 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site