linux-kernel - Re: [PATCH 4/8] ASoC: amd: acp: Add generic machine driver support for ACP cards

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YWBoc4LJPUS733ee@sirena.org.uk>
Date:   Fri, 8 Oct 2021 16:49:07 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Ajit Kumar Pandey <AjitKumar.Pandey@....com>
Cc:     alsa-devel@...a-project.org, Vijendar.Mukunda@....com,
        Basavaraj.Hiregoudar@....com, Sunil-kumar.Dommati@....com,
        Alexander.Deucher@....com, Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/8] ASoC: amd: acp: Add generic machine driver support
 for ACP cards

On Thu, Sep 30, 2021 at 06:54:14PM +0530, Ajit Kumar Pandey wrote:

A couple of things here, most of these are probably fine for now
other than the EXPORT_SYMBOL but I think you're likely to run
into issues going forward and need to refactor.

> +	switch (drvdata->hs_codec_id) {
> +	case RT5682:
> +		pll_id = RT5682_PLL2;
> +		pll_src = RT5682_PLL2_S_MCLK;
> +		freq_in = PCO_PLAT_CLK;
> +		freq_out = RT5682_PLL_FREQ;
> +		clk_id = RT5682_SCLK_S_PLL2;
> +		clk_freq = RT5682_PLL_FREQ;
> +		wclk_name = "rt5682-dai-wclk";
> +		bclk_name = "rt5682-dai-bclk";
> +		drvdata->dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
> +				   | SND_SOC_DAIFMT_CBP_CFP;
> +		snd_soc_dapm_add_routes(&rtd->card->dapm, rt5682_map, ARRAY_SIZE(rt5682_map));
> +		break;

It feels like this is going to run into scaling issues going
forward and you're likely to need separate operations for
different CODECs rather than just different IDs.  Similar issues
apply for the amps, it feels like you want to be passing separate
ops in rather than having these switch statements.

> +	/* Do nothing for dummy codec */
> +	if (!drvdata->hs_codec_id && drvdata->amp_codec_id)
> +		return;

Wha the test seems to say is do nothing if there's no CODEC but
there is an amp...

> +
> +	clk_disable_unprepare(drvdata->wclk);
> +}

...though I'd expect that given that the clock API accepts NULL
clocks you could just remove these checks and unconditionally use
the clocks.

> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(acp_legacy_dai_links_create, SND_SOC_AMD_MACH);

EXPORT_SYMBOL_GPL_NS() - ASoC is all EXPORT_SYMBOL_GPL.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)