Re: [PATCH v7 18/22] ASoC: qdsp6: audioreach: add topology support - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	bjorn.andersson@linaro.org, broonie@kernel.org, robh@kernel.org
Cc: plai@codeaurora.org, tiwai@suse.de, devicetree@vger.kernel.org,
	perex@perex.cz, alsa-devel@alsa-project.org, lgirdwood@gmail.com,
	bgoswami@codeaurora.org
Subject: Re: [PATCH v7 18/22] ASoC: qdsp6: audioreach: add topology support
Date: Wed, 22 Sep 2021 10:40:16 -0500	[thread overview]
Message-ID: <c03cedad-971d-2eee-a1a5-7f2d6c105682@linux.intel.com> (raw)
In-Reply-To: <9ea759db-f2ae-8b26-eff9-8557267803d2@linaro.org>

>>> +    /* Inline with Qualcomm UCM configs and linux-firmware path */
>>> +    snprintf(tplg_fw_name, sizeof(tplg_fw_name),
>>> "qcom/%s/%s-tplg.bin", card->driver_name,
>>> +         card->name);
>>
>> use kasprintf instead of fixed 128-char array?
> I moved this to kasprintf in next version.
> 
>>
>> Also you should use a qcom/audioreach/ prefix to possible interference
>> with other parts of qcom...
>>
> 
> So Qualcomm linux-firmwares are arranged something like
> 
> qcom/sdm845/*
> qcom/sm8250/*
> qcom/sm8150/*
> 
> and UCM something like
> this:Qualcomm/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0.conf
> 
> Qualcomm/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0.conf
> 
> 
> Atleast in Qualcomm soundcard case we have driver name set to SoC name
> and we tend to reuse this driver across multiple platforms.
> 
> second part card name actually is from model device tree property, in
> this case which is "Qualcomm-RB5-WSA8815-Speakers-DMIC0"
> 
> so we will endup looking for
> /lib/firmare/qcom/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0-tplg.bin
> 
> AFAIU, it should not interface with any other qcom parts.
> 
> for Other qcom parts this model will change so the topology file name.

that should be fine.

>>> +
>>> +    ret = request_firmware(&fw, tplg_fw_name, dev);
>>> +    if (ret < 0) {
>>> +        dev_info(dev, "loading %s failed %d, falling back to
>>> dfw-audioreach-tplg.bin\n",
>>> +             tplg_fw_name, ret);
>>> +        /* default firmware */
>>> +        ret = request_firmware(&fw, "dfw-audioreach-tplg.bin", dev);
>>> +        if (ret < 0) {
>>> +            dev_err(dev, "tplg fw dfw-audioreach-tplg.bin load
>>> failed with %d\n", ret);
>>
>> the dfw prefix isn't very helpful...Intel's example of "dfw_sst.bin" is
>> a historical reference, not something you should reuse.
> 
> Rethinking on this once again, Am not sure if it even makes sense to
> support this default setup. It will be very hard to get a working
> defalut tplg on every platform. So am planning to remove this in next
> version.
> 
> Do you see any issues?

No, I don't think it's practical to use this default name either. We
don't have it for SOF.

Even for developers who modify a default, it's better to place a new
topology with the same name in /lib/firmware/updates/qcom and let
request_firmware() find the updated file with the existing paths [1]

[1]
https://www.kernel.org/doc/html/latest/driver-api/firmware/fw_search_path.html