Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a71c4a2c-06f9-faa7-07ee-783ee7f136ec@linux.intel.com>
Date:   Wed, 11 May 2022 09:02:58 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [PATCH] [v2] ASoC: Intel: sof_cs42l42: adding support for ADL
 configuration and BT offload audio
Content-Language: en-US
To:     Terry Chen <terry_chen@wistron.corp-partner.google.com>
Cc:     alsa-devel@alsa-project.org, cezary.rojewski@intel.com,
        liam.r.girdwood@linux.intel.com, yang.jie@linux.intel.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        brent.lu@intel.com, cujomalainey@chromium.org,
        Sean Paul <seanpaul@chromium.org>, casey.g.bowman@intel.com,
        Mark Hsieh <mark_hsieh@wistron.corp-partner.google.com>,
        vamshi.krishna.gopal@intel.com, Mac Chiang <mac.chiang@intel.com>,
        kai.vehmanen@linux.intel.com, linux-kernel@vger.kernel.org
References: <20220510104829.1466968-1-terry_chen@wistron.corp-partner.google.com>
 <190c9add-7fa4-8e76-bfcb-43d30f22f8d9@linux.intel.com>
 <CAMmR3bFad5ODKYUCg8Tp8GVk__AdaQHcpLnRmFyAGXu8Wpycog@mail.gmail.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <CAMmR3bFad5ODKYUCg8Tp8GVk__AdaQHcpLnRmFyAGXu8Wpycog@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 5/11/22 01:33, Terry Chen wrote:
> Hi Pierre-Louis
> 
>> @@ -522,6 +578,14 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev,
>>                               goto devm_err;
>>                       }
>>                       break;
>> +             case LINK_BT:
>> +                     ret = create_bt_offload_dai_links(dev, links, cpus, &id, ssp_bt);
>> +                     if (ret < 0) {
>> +                             dev_err(dev, "fail to create bt offload dai links, ret %d\n",
>> +                                     ret);
> 
> For this point, we just follow Intel member to write for this coding
> style. The other component also was the same style.

the magic of copy-paste, eh? Please update this, thanks.

>     > @@ -384,6 +384,14 @@ struct snd_soc_acpi_mach
>     snd_soc_acpi_intel_adl_machines[] = {
>     >               .sof_fw_filename = "sof-adl.ri",
>     >               .sof_tplg_filename = "sof-adl-cs35l41.tplg",
>     >       },
>     > +     {
>     > +             .id = "10134242",
>     > +             .drv_name = "adl_mx98360a_cs4242",
>     > +             .machine_quirk = snd_soc_acpi_codec_list,
>     > +             .quirk_data = &adl_max98360a_amp,
>     > +             .sof_fw_filename = "sof-adl.ri",
> 
>     This  also was the same style with others.

No, it's not a matter of style but rather that this field was *REMOVED*,
this cannot possibly compile.

see commit a6264056b39ee ("ASoC: soc-acpi: remove sof_fw_filename
")

If you had submitted this patch through the SOF tree, you would have
seen a compilation error.

> 
>     > +             .sof_tplg_filename = "sof-adl-max98360a-rt5682.tplg",
> 
>     Why would you refer to a topology that uses a different codec?
> 
> 
>  Because Intel college use the same naming style for the same audio codec.

It's bad practice to use the same topology name for different platforms
based on different codecs. One evolution of the topology would impact an
unrelated platform. Please use a symlink or duplicate the topology with
a different name, this is not future-proof and will be problematic for
releases.