linux-kernel - Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p

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: <Yi6zlIvA6t+yEbza@google.com>
Date:   Mon, 14 Mar 2022 11:16:36 +0800
From:   Tzung-Bi Shih <tzungbi@...nel.org>
To:     Jiaxin Yu <jiaxin.yu@...iatek.com>
Cc:     broonie@...nel.org, robh+dt@...nel.org, aaronyu@...gle.com,
        matthias.bgg@...il.com, trevor.wu@...iatek.com, linmq006@...il.com,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s

On Sat, Mar 12, 2022 at 12:22:13AM +0800, Jiaxin Yu wrote:
> Supports machines with rt1015p and rt5682s. Uses new proposed compatible
> string "mt8192_mt6359_rt1015p_rt5682s". Using define to simplifies card
> name and compatible name, and uses the snd_soc_of_get_dai_link_codecs()
> to complete the configuration of dai_link's codecs.

Rephrase the paragraph and consider to use imperative mood as [1] suggests.

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

The patch includes multiple changes.  Could you further separate it into:
1. Refactor for I2S3 DAI links for original amps.
2. Refactor for I2S8 and I2S9 DAI links for original headset codec RT5682.
3. Support mt8192_mt6359_rt1015p_rt5682s for RT5682S.

> Reported-by: kernel test robot <lkp@...el.com>

The tag can be dropped as "Supports machines with rt1015p and rt5682s" is not
an idea from the bot.  Put it in change logs if you would like it to be a
record.

> +static int mt8192_mt6359_card_set_be_link(struct snd_soc_card *card,
> +					  struct snd_soc_dai_link *link,
> +					  struct device_node *node,
> +					  char *link_name)
> +{
> +	int ret;
> +
> +	if (node && strcmp(link->name, link_name) == 0) {
> +		ret = snd_soc_of_get_dai_link_codecs(card->dev, node, link);
> +		if (ret < 0) {
> +			dev_err(card->dev, "get dai link codecs fail\n");

How about using dev_err_probe()?  As the callers also report errors if
mt8192_mt6359_card_set_be_link() fails, the log can be dropped.

>  static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
>  {
>  	struct snd_soc_card *card;
> -	struct device_node *platform_node, *hdmi_codec;
> +	struct device_node *platform_node, *hdmi_codec, *headset_codec, *speaker_codec;
>  	int ret, i;
>  	struct snd_soc_dai_link *dai_link;
>  	struct mt8192_mt6359_priv *priv;
> +	struct device *dev;
[...]
> +	dev = &pdev->dev;

I don't think it could help too much unless it was struggling for code
columns.  Please consider to drop the variable or you should s/&pdev->dev/dev/g
for the function.

> +	speaker_codec = of_get_child_by_name(dev->of_node, "mediatek,speaker-codec");
> +	if (!speaker_codec) {
> +		ret = -EINVAL;
> +		dev_err_probe(dev, ret, "Property 'speaker_codec' missing or invalid\n");
> +		goto err_speaker_codec;
> +	}
> +
> +	headset_codec = of_get_child_by_name(dev->of_node, "mediatek,headset-codec");
> +	if (!headset_codec) {
> +		ret = -EINVAL;
> +		dev_err_probe(dev, ret, "Property 'headset_codec' missing or invalid\n");
> +		goto err_headset_codec;
> +	}

Are these new DT bindings?  They are not in
Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml.

> +		ret = mt8192_mt6359_card_set_be_link(card, dai_link, speaker_codec, "I2S3");
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "%s set speaker_codec fail\n",

As mentioned above, I don't think `dev` could help too much.  However, to be
consistent, either drop the `dev` variable or use `dev` here.

> +		ret = mt8192_mt6359_card_set_be_link(card, dai_link, headset_codec, "I2S8");
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "%s set headset_codec fail\n",
> +				      dai_link->name);

Ditto.

> +		ret = mt8192_mt6359_card_set_be_link(card, dai_link, headset_codec, "I2S9");
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "%s set headset_codec fail\n",
> +				      dai_link->name);

Ditto.

>  	ret = mt8192_afe_gpio_init(&pdev->dev);
>  	if (ret) {
> -		dev_err(&pdev->dev, "init gpio error %d\n", ret);
> -		goto put_hdmi_codec;
> +		dev_err_probe(&pdev->dev, ret, "%s init gpio error\n", __func__);

Ditto.

>  	ret = devm_snd_soc_register_card(&pdev->dev, card);
> -
> -put_hdmi_codec:
> +	if (ret)
> +		dev_err_probe(&pdev->dev, ret,

Ditto.

> +err_probe:
> +	of_node_put(headset_codec);
> +err_headset_codec:
> +	of_node_put(speaker_codec);
> +err_speaker_codec:
>  	of_node_put(hdmi_codec);
> -put_platform_node:
> +err_hdmi_codec:
>  	of_node_put(platform_node);
> +err_platform_node:
> +

The new line can be dropped.