RE: [PATCH] ASoC: hdac_hda: add HDA patch loader support

20 Sep 2023

...
-----Original Message-----
From: Takashi Iwai tiwai@suse.de
Sent: Tuesday, September 19, 2023 8:55 PM
To: Bard Liao yung-chuan.liao@linux.intel.com
Cc: broonie@kernel.org; alsa-devel@alsa-project.org; pierre-
louis.bossart@linux.intel.com; Liao, Bard bard.liao@intel.com;
peter.ujfalusi@linux.intel.com; kai.vehmanen@linux.intel.com
Subject: Re: [PATCH] ASoC: hdac_hda: add HDA patch loader support
On Tue, 19 Sep 2023 10:32:09 +0200,
Bard Liao wrote:
...
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+static char *loadable_patch[SNDRV_CARDS];
Hm, this size looks wrong.
In the code later in this patch, dev_index points to the codec
address, and it's basically irrelevant with SNDRV_CARDS.  As
SNDRV_CARDS might be 8 depending on the config, it can go beyond the
array size, too.  Use a different array size to match with the codec
address ranges.
Thanks for pointing this out. Indeed, the array size is irrelevant with
SNDRV_CARDS. I will change it to HDA_MAX_CODECS as it is the
available number that we probe the codec.
...
...
+module_param_array_named(patch, loadable_patch, charp, NULL, 0444);
+MODULE_PARM_DESC(patch, "Patch file for Intel HD audio interface.");
Better to give a bit more description; when it's a codec address
array, it can be  non-zero, and user may wonder why it's not applied.
Yes, I will do it.
...
...
+#ifdef CONFIG_SND_HDA_PATCH_LOADER

if (loadable_patch[hda_pvt->dev_index] &&

*loadable_patch[hda_pvt->dev_index]) {
...

dev_info(&hdev->dev, "Applying patch firmware '%s'\n",


	 loadable_patch[hda_pvt->dev_index]);


ret = request_firmware(&hda_pvt->fw,



loadable_patch[hda_pvt->dev_index],
...

		       &hdev->dev);


if (ret < 0)


	goto error_no_pm;


if (hda_pvt->fw) {


	ret = snd_hda_load_patch(hcodec->bus, hda_pvt-



fw->size, hda_pvt->fw->data);

	if (ret < 0) {


		dev_err(&hdev->dev, "failed to load hda patch



%d\n", ret);
...

		goto error_no_pm;


	}


	release_firmware(hda_pvt->fw);


	hda_pvt->fw = NULL;



So the hda_pvt->fw is only for a temporary use, already released
already here.  Then...
...

--- a/sound/soc/codecs/hdac_hda.h
+++ b/sound/soc/codecs/hdac_hda.h
@@ -26,6 +26,10 @@ struct hdac_hda_priv {
   struct hda_codec *codec;
   struct hdac_hda_pcm pcm[HDAC_DAI_ID_NUM];
   bool need_display_power;

int dev_index;

+#ifdef CONFIG_SND_HDA_PATCH_LOADER

const struct firmware *fw;

+#endif
... we don't have to add a new extra field, right?
Yes, I will remove it in the follow up patch.
Thanks,
Bard
...
thanks,
Takashi

    