From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9C433C433F5 for ; Mon, 14 Mar 2022 03:17:46 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 0CFE816A5; Mon, 14 Mar 2022 04:16:54 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 0CFE816A5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1647227864; bh=pqkDNAeAEYNvG49k/Hx0tByN0ecvsp2DdmQ6if+3LdA=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=g0Rogwc1Lc4YM9OUNS8LhTq/rV87sXZBgKbCTJU8x9EoFtlhr5t58JuwoHszvDh82 quy7Wz1VY8TgDW4JZmuscOd5T+mhhoQZIGbQnp2G9Oo6/yU236m0xJWX756DZSVugx QNwJWwl/h6KvNEIXJ+MHMvMhaU7CSM/vRZRCReik= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 8FF69F8012C; Mon, 14 Mar 2022 04:16:53 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id BCD33F80139; Mon, 14 Mar 2022 04:16:51 +0100 (CET) Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 5A742F8011C for ; Mon, 14 Mar 2022 04:16:45 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 5A742F8011C Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="K6stHjXB" Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4F62D60F7C; Mon, 14 Mar 2022 03:16:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C70CC340E9; Mon, 14 Mar 2022 03:16:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1647227801; bh=pqkDNAeAEYNvG49k/Hx0tByN0ecvsp2DdmQ6if+3LdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=K6stHjXByT/LIIEPIIN7gV5+ue1kVhi02UDo/UKc/m29vF0fX3xut4Yg4viAqkjQ6 1xe5srH6/IkKVbXiIHrX2MrjP8vuOu03F5X+2fbduJUhQ7Fnk1h9me4H3aqfbCngOp dxsGOek5ppdJBELp7JOkg0DA8kH+UwxOsJ9fdy27kGaHlG8nEHE8jQIV+W4X8tUxOg azJ/56yD5EYylptV+CI2SzgAZhq4poAfP/p1mNf5P3Vk5pqFwi56V3pfdYEm4pX+aN Y1vEaMqAca9yOZE7ajA25CxUIDKZDeJH6MVkyab2wvikK3vpXfRqig5ckIY6hTAcJj fR4pOtOJOq5Qw== Date: Mon, 14 Mar 2022 11:16:36 +0800 From: Tzung-Bi Shih To: Jiaxin Yu Subject: Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s Message-ID: References: <20220311162213.6942-1-jiaxin.yu@mediatek.com> <20220311162213.6942-3-jiaxin.yu@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220311162213.6942-3-jiaxin.yu@mediatek.com> Cc: devicetree@vger.kernel.org, linmq006@gmail.com, kernel test robot , alsa-devel@alsa-project.org, robh+dt@kernel.org, linux-kernel@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com, broonie@kernel.org, linux-mediatek@lists.infradead.org, trevor.wu@mediatek.com, matthias.bgg@gmail.com, aaronyu@google.com, linux-arm-kernel@lists.infradead.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 354F5C433EF for ; Mon, 14 Mar 2022 03:17:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=TwCnu2ScxgA2PF4A+X9gEAHFscj0+fxnyFqXI/pDt+E=; b=1xDzuWhCI03arN NlWqCiE5HoGCycF4XNhkmVgrqp5eCNtGxu76GkgNnWzWzyYkGoBpOrm3ronv2OOp/LPJa4h5Ck6jx h/8ay8RbMESHQeLmRDl2mdMTzrFyYMc9ao1uKR7/fQhF95iOA41fUxT2IUTxZNGOJzhk03+U1RdAK LCD6FZ3UMGT7vR8/QFOCpWhaooJdRESIL4NYLFw1hcj4bIsXhQj5mln4BX9XxgvQ7sO7aX2JPCQQT OkXxrZRV60yn/+5CRK7JdwJ8Qq94iTzFSNeW4QxkODjRB8zhqGNP4KPLAWSpAn7frXcOz/QCTonbe xPFqvvKLhcvfkQy+e22w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nTbCZ-003iBu-NN; Mon, 14 Mar 2022 03:16:55 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nTbCN-003iAz-Qd; Mon, 14 Mar 2022 03:16:45 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4F62D60F7C; Mon, 14 Mar 2022 03:16:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C70CC340E9; Mon, 14 Mar 2022 03:16:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1647227801; bh=pqkDNAeAEYNvG49k/Hx0tByN0ecvsp2DdmQ6if+3LdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=K6stHjXByT/LIIEPIIN7gV5+ue1kVhi02UDo/UKc/m29vF0fX3xut4Yg4viAqkjQ6 1xe5srH6/IkKVbXiIHrX2MrjP8vuOu03F5X+2fbduJUhQ7Fnk1h9me4H3aqfbCngOp dxsGOek5ppdJBELp7JOkg0DA8kH+UwxOsJ9fdy27kGaHlG8nEHE8jQIV+W4X8tUxOg azJ/56yD5EYylptV+CI2SzgAZhq4poAfP/p1mNf5P3Vk5pqFwi56V3pfdYEm4pX+aN Y1vEaMqAca9yOZE7ajA25CxUIDKZDeJH6MVkyab2wvikK3vpXfRqig5ckIY6hTAcJj fR4pOtOJOq5Qw== Date: Mon, 14 Mar 2022 11:16:36 +0800 From: Tzung-Bi Shih To: Jiaxin Yu Cc: broonie@kernel.org, robh+dt@kernel.org, aaronyu@google.com, matthias.bgg@gmail.com, trevor.wu@mediatek.com, linmq006@gmail.com, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com, kernel test robot Subject: Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s Message-ID: References: <20220311162213.6942-1-jiaxin.yu@mediatek.com> <20220311162213.6942-3-jiaxin.yu@mediatek.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220311162213.6942-3-jiaxin.yu@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220313_201643_991480_2B2E2E2C X-CRM114-Status: GOOD ( 21.16 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org 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 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. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 43A7CC433F5 for ; Mon, 14 Mar 2022 03:18:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=M2Bike1IsuV4yEUnSMtkdMAcjDEzrgncmqvk4SLfWd4=; b=x8KSKDLtN8EttO R5WiQHGq2ApCxtfc3Ug3YClDbEYG+wT3GlC4LplEtfQW/P5jCuqaSN1vDrf0DoccAgcPJX66M+Mi2 GBou9fcDXaoMYdkSmZLGl3dAEL+pg7UKCDq/NdkMVYYeB6sgqMsvrKdAVPDAi7a4X0uNyEAdaW/TQ IamxVQp7H6P0l6e8BwXjkdNseXHffd/2fc1y6nzwClIJLA1n8uj2YHdAqe9hbAi3XNMAhf1sxLaoC F3ckdkWh+TaZg1alMu2MFMA/4hqtOKuR1vg1jkYJd1siflBZXkHYaFVFOhczIwEfAfk+0cQqDO2N8 yoy9PeO0Pjai+DGXZs4g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nTbCR-003iBR-Lk; Mon, 14 Mar 2022 03:16:47 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nTbCN-003iAz-Qd; Mon, 14 Mar 2022 03:16:45 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4F62D60F7C; Mon, 14 Mar 2022 03:16:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C70CC340E9; Mon, 14 Mar 2022 03:16:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1647227801; bh=pqkDNAeAEYNvG49k/Hx0tByN0ecvsp2DdmQ6if+3LdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=K6stHjXByT/LIIEPIIN7gV5+ue1kVhi02UDo/UKc/m29vF0fX3xut4Yg4viAqkjQ6 1xe5srH6/IkKVbXiIHrX2MrjP8vuOu03F5X+2fbduJUhQ7Fnk1h9me4H3aqfbCngOp dxsGOek5ppdJBELp7JOkg0DA8kH+UwxOsJ9fdy27kGaHlG8nEHE8jQIV+W4X8tUxOg azJ/56yD5EYylptV+CI2SzgAZhq4poAfP/p1mNf5P3Vk5pqFwi56V3pfdYEm4pX+aN Y1vEaMqAca9yOZE7ajA25CxUIDKZDeJH6MVkyab2wvikK3vpXfRqig5ckIY6hTAcJj fR4pOtOJOq5Qw== Date: Mon, 14 Mar 2022 11:16:36 +0800 From: Tzung-Bi Shih To: Jiaxin Yu Cc: broonie@kernel.org, robh+dt@kernel.org, aaronyu@google.com, matthias.bgg@gmail.com, trevor.wu@mediatek.com, linmq006@gmail.com, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com, kernel test robot Subject: Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s Message-ID: References: <20220311162213.6942-1-jiaxin.yu@mediatek.com> <20220311162213.6942-3-jiaxin.yu@mediatek.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220311162213.6942-3-jiaxin.yu@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220313_201643_991480_2B2E2E2C X-CRM114-Status: GOOD ( 21.16 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C79A0C433EF for ; Mon, 14 Mar 2022 03:17:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235783AbiCNDS0 (ORCPT ); Sun, 13 Mar 2022 23:18:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49452 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229452AbiCNDRv (ORCPT ); Sun, 13 Mar 2022 23:17:51 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9D9015A31; Sun, 13 Mar 2022 20:16:42 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4E40260F0A; Mon, 14 Mar 2022 03:16:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C70CC340E9; Mon, 14 Mar 2022 03:16:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1647227801; bh=pqkDNAeAEYNvG49k/Hx0tByN0ecvsp2DdmQ6if+3LdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=K6stHjXByT/LIIEPIIN7gV5+ue1kVhi02UDo/UKc/m29vF0fX3xut4Yg4viAqkjQ6 1xe5srH6/IkKVbXiIHrX2MrjP8vuOu03F5X+2fbduJUhQ7Fnk1h9me4H3aqfbCngOp dxsGOek5ppdJBELp7JOkg0DA8kH+UwxOsJ9fdy27kGaHlG8nEHE8jQIV+W4X8tUxOg azJ/56yD5EYylptV+CI2SzgAZhq4poAfP/p1mNf5P3Vk5pqFwi56V3pfdYEm4pX+aN Y1vEaMqAca9yOZE7ajA25CxUIDKZDeJH6MVkyab2wvikK3vpXfRqig5ckIY6hTAcJj fR4pOtOJOq5Qw== Date: Mon, 14 Mar 2022 11:16:36 +0800 From: Tzung-Bi Shih To: Jiaxin Yu Cc: broonie@kernel.org, robh+dt@kernel.org, aaronyu@google.com, matthias.bgg@gmail.com, trevor.wu@mediatek.com, linmq006@gmail.com, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com, kernel test robot Subject: Re: [v4 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s Message-ID: References: <20220311162213.6942-1-jiaxin.yu@mediatek.com> <20220311162213.6942-3-jiaxin.yu@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220311162213.6942-3-jiaxin.yu@mediatek.com> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 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.