Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 13/14] ASoC: Intel: bytcr_wm5102: Add machine driver for
 BYT/WM5102
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     Lee Jones <lee.jones@linaro.org>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Cezary Rojewski <cezary.rojewski@intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Jie Yang <yang.jie@linux.intel.com>,
        Mark Brown <broonie@kernel.org>, patches@opensource.cirrus.com,
        linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org
References: <20201227211232.117801-1-hdegoede@redhat.com>
 <20201227211232.117801-14-hdegoede@redhat.com>
 <20201229135836.GO9673@ediswmail.ad.cirrus.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <6d881b08-2511-5dcf-0f88-4f54b937c967@redhat.com>
Date:   Sat, 16 Jan 2021 17:49:13 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20201229135836.GO9673@ediswmail.ad.cirrus.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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