> -----Original Message-----
> From: Takashi Iwai <tiwai@xxxxxxx>
> Sent: Tuesday, September 19, 2023 8:55 PM
> To: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> Cc: broonie@xxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; Liao, Bard <bard.liao@xxxxxxxxx>;
> peter.ujfalusi@xxxxxxxxxxxxxxx; kai.vehmanen@xxxxxxxxxxxxxxx
> 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
