linux-kernel - Re: [v1 2/2] ASoC: mediatek: mt8188-mt6359: add rt5682s support

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <e0cf4d2a6ccb0661e48a9fa744fb8d22559251b2.camel@mediatek.com>
Date:   Mon, 25 Sep 2023 09:42:03 +0000
From:   Trevor Wu (吳文良) <Trevor.Wu@...iatek.com>
To:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "xiazhengqiao@...qin.corp-partner.google.com" 
        <xiazhengqiao@...qin.corp-partner.google.com>,
        "perex@...ex.cz" <perex@...ex.cz>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [v1 2/2] ASoC: mediatek: mt8188-mt6359: add rt5682s support

On Mon, 2023-09-25 at 16:38 +0800, xiazhengqiao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  To use RT5682S as the codec and MAX98390 as the amp, add a new
> sound card named mt8188_rt5682.
> 
> Signed-off-by: xiazhengqiao <
> xiazhengqiao@...qin.corp-partner.google.com>
> ---
>  sound/soc/mediatek/Kconfig                |   1 +
>  sound/soc/mediatek/mt8188/mt8188-mt6359.c | 154
> +++++++++++++++++++++-
>  2 files changed, 154 insertions(+), 1 deletion(-)
> 
> 

..snip..

>  
> +static int mt8186_rt5682s_i2s_hw_params(struct snd_pcm_substream 


You are using 'rt5682s' here, but in other functions you are using
'rt5682'. Please ensure consistent naming to avoid confusion.

> *substream,
> +					struct snd_pcm_hw_params
> *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +	unsigned int rate = params_rate(params);
> +	int bitwidth;
> +	int ret;
> +
> +	bitwidth = snd_pcm_format_width(params_format(params));
> +	if (bitwidth < 0) {
> +		dev_err(card->dev, "invalid bit width: %d\n",
> bitwidth);
> +		return bitwidth;
> +	}
> +
> +	ret = snd_soc_dai_set_tdm_slot(codec_dai, 0x00, 0x0, 0x2,
> bitwidth);
> +	if (ret) {
> +		dev_err(card->dev, "failed to set tdm slot\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL1,
> RT5682_PLL1_S_BCLK1,
> +				  rate * 32, rate * 512);
> +	if (ret) {
> +		dev_err(card->dev, "failed to set pll\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,
> +				     rate * 512, SND_SOC_CLOCK_IN);
> +	if (ret) {
> +		dev_err(card->dev, "failed to set sysclk\n");
> +		return ret;
> +	}
> +
> +	return snd_soc_dai_set_sysclk(cpu_dai, 0, rate * 128,
> +				      SND_SOC_CLOCK_OUT);
> +}
> +
> +static const struct snd_soc_ops mt8188_rt5682s_i2s_ops = {
> +	.hw_params = mt8186_rt5682s_i2s_hw_params,
> +};
> +
>  static int mt8188_sof_be_hw_params(struct snd_pcm_substream
> *substream,
>  				   struct snd_pcm_hw_params *params)
>  {
> @@ -1163,6 +1286,21 @@ static void mt8188_fixup_controls(struct
> snd_soc_card *card)
>  			snd_ctl_remove(card->snd_card, kctl);
>  		else
>  			dev_warn(card->dev, "Cannot find ctl :
> Headphone Switch\n");
> +	} else if (card_data->quirk & RT5682_HS_PRESENT) {
> +		struct snd_soc_dapm_widget *w, *next_w;
> +
> +		for_each_card_widgets_safe(card, w, next_w) {
> +			if (strcmp(w->name, "Headphone"))
> +				continue;
> +
> +			snd_soc_dapm_free_widget(w);
> +		}
> +
> +		kctl = ctl_find(card->snd_card, "Headphone Switch");
> +		if (kctl)
> +			snd_ctl_remove(card->snd_card, kctl);
> +		else
> +			dev_warn(card->dev, "Cannot find ctl :
> Headphone Switch\n");

It seems that the implementation is the same as NAU8825_HS_PRESENT.
Maybe you can merge them as shown below.

if(quirk & RT5682s || quirk & NAU8825) {

	// action here
}

Thanks,
Trevor

>  	}
>  }
>  
> @@ -1190,6 +1328,7 @@ static int mt8188_mt6359_dev_probe(struct
> platform_device *pdev)
>  	struct snd_soc_dai_link *dai_link;
>  	bool init_mt6359 = false;
>  	bool init_nau8825 = false;
> +	bool init_rt5682 = false;
>  	bool init_max98390 = false;
>  	bool init_dumb = false;
>  	int ret, i;
> @@ -1306,6 +1445,13 @@ static int mt8188_mt6359_dev_probe(struct
> platform_device *pdev)
>  					dai_link->exit =
> mt8188_nau8825_codec_exit;
>  					init_nau8825 = true;
>  				}
> +			} else if (!strcmp(dai_link->codecs->dai_name,
> RT5682_CODEC_DAI)) {
> +				dai_link->ops =
> &mt8188_rt5682s_i2s_ops;
> +				if (!init_rt5682) {
> +					dai_link->init =
> mt8188_rt5682_codec_init;
> +					dai_link->exit =
> mt8188_rt5682_codec_exit;
> +					init_rt5682 = true;
> +				}
>  			} else {
>  				if (strcmp(dai_link->codecs->dai_name,
> "snd-soc-dummy-dai")) {
>  					if (!init_dumb) {
> @@ -1343,9 +1489,15 @@ static struct mt8188_card_data
> mt8188_nau8825_card = {
>  	.quirk = NAU8825_HS_PRESENT,
>  };
>  
> +static struct mt8188_card_data mt8188_rt5682_card = {
> +	.name = "mt8188_rt5682",
> +	.quirk = RT5682_HS_PRESENT,
> +};
> +
>  static const struct of_device_id mt8188_mt6359_dt_match[] = {
>  	{ .compatible = "mediatek,mt8188-mt6359-evb", .data =
> &mt8188_evb_card, },
>  	{ .compatible = "mediatek,mt8188-nau8825", .data =
> &mt8188_nau8825_card, },
> +	{ .compatible = "mediatek,mt8188-rt5682", .data =
> &mt8188_rt5682_card, },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, mt8188_mt6359_dt_match);
> -- 
> 2.17.1