Re: [PATCH v2 1/2] ASoC: codecs: nau8824: Add master clock handling

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/06/2024 08:17, Maxim Kochetkov wrote:
> Use master clock "mclk" if provided through device tree.
> 
> Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
> ---
>  sound/soc/codecs/nau8824.c | 21 +++++++++++++++++++--
>  sound/soc/codecs/nau8824.h |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/nau8824.c b/sound/soc/codecs/nau8824.c
> index f92b95b21cae..7153b746f4b1 100644
> --- a/sound/soc/codecs/nau8824.c
> +++ b/sound/soc/codecs/nau8824.c
> @@ -520,8 +520,15 @@ static int system_clock_control(struct snd_soc_dapm_widget *w,
>  		} else {
>  			nau8824_config_sysclk(nau8824, NAU8824_CLK_DIS, 0);
>  		}
> +
> +		if (!IS_ERR(nau8824->mclk))
> +			clk_disable_unprepare(nau8824->mclk);
>  	} else {
>  		dev_dbg(nau8824->dev, "system clock control : POWER ON\n");
> +
> +		if (!IS_ERR(nau8824->mclk))

Nah, clock API is not used like this. Open clk_prepare_enable() and look
how it handles NULL ptr.

> +			clk_prepare_enable(nau8824->mclk);
> +
>  		/* Check the clock source setting is proper or not
>  		 * no matter the source is from FLL or MCLK.
>  		 */
> @@ -563,16 +570,22 @@ static int dmic_clock_control(struct snd_soc_dapm_widget *w,
>  	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>  	struct nau8824 *nau8824 = snd_soc_component_get_drvdata(component);
>  	int src;
> +	unsigned int freq;
> +
> +	if (!IS_ERR(nau8824->mclk))
> +		freq = clk_get_rate(nau8824->mclk);
> +	else
> +		freq = nau8824->fs * 256;
>  
>  	/* The DMIC clock is gotten from system clock (256fs) divided by
>  	 * DMIC_SRC (1, 2, 4, 8, 16, 32). The clock has to be equal or
>  	 * less than 3.072 MHz.
>  	 */
>  	for (src = 0; src < 5; src++) {
> -		if ((0x1 << (8 - src)) * nau8824->fs <= DMIC_CLK)
> +		if (freq / (0x1 << src) <= DMIC_CLK)
>  			break;
>  	}
> -	dev_dbg(nau8824->dev, "dmic src %d for mclk %d\n", src, nau8824->fs * 256);
> +	dev_dbg(nau8824->dev, "dmic src %d for mclk %d\n", src, freq);
>  	regmap_update_bits(nau8824->regmap, NAU8824_REG_CLK_DIVIDER,
>  		NAU8824_CLK_DMIC_SRC_MASK, (src << NAU8824_CLK_DMIC_SRC_SFT));
>  
> @@ -1871,6 +1884,10 @@ static int nau8824_read_device_properties(struct device *dev,
>  	if (ret)
>  		nau8824->jack_eject_debounce = 1;
>  
> +	nau8824->mclk = devm_clk_get(dev, "mclk");
> +	if (PTR_ERR(nau8824->mclk) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

Aren't you open-coding getting optional clock?

Best regards,
Krzysztof