Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 14 Dec 2022 13:31:36 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Lukasz Majewski <lukma@denx.de>
CC:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, Stephen Kitt <steve@sk2.org>,
        <patches@opensource.cirrus.com>, <alsa-devel@alsa-project.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks
Message-ID: <20221214133136.GQ105268@ediswmail.ad.cirrus.com>
References: <20221214123743.3713843-1-lukma@denx.de>
 <20221214123743.3713843-3-lukma@denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221214123743.3713843-3-lukma@denx.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Wed, Dec 14, 2022 at 01:37:41PM +0100, Lukasz Majewski wrote:
> Without this change, the wm8940 driver is not working when
> set_sysclk callback (wm8940_set_dai_sysclk) is called with
> frequency not listed in the switch clause.
> 
> This change adjusts this driver to allow non-standard frequency
> set (just after the boot) being adjusted afterwards by the sound
> system core code.
> 
> Moreover, support for internal wm8940's PLL is provided, so it
> can generate clocks when HOST system is not able to do it.
> 
> Code in this commit is based on previous change done for wm8974
> (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  	struct snd_soc_component *component = dai->component;
> +	struct wm8940_priv *priv = snd_soc_component_get_drvdata(component);
>  	u16 iface = snd_soc_component_read(component, WM8940_IFACE) & 0xFD9F;
>  	u16 addcntrl = snd_soc_component_read(component, WM8940_ADDCNTRL) & 0xFFF1;
>  	u16 companding =  snd_soc_component_read(component,
>  						WM8940_COMPANDINGCTL) & 0xFFDF;
>  	int ret;
>  
> +	priv->fs = params_rate(params);
> +	ret = wm8940_update_clocks(dai);
> +	if (ret)
> +		return ret;
> +

I think this all looks mostly good, my only slight concern would
be the interaction with the manual functions for settings the PLL
etc. I guess under this code, whatever manual settings were
configured will be overwritten with the new auto settings, I
think this should be fine as the PLL wants to be run in a pretty
narrow band anyway, so the settings are likely identical. Do you
have any thoughts?

Thanks,
Charles