| Message ID | 20190822190425.23001-31-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Clenaup SST initialization | expand |
On 8/22/19 2:04 PM, Cezary Rojewski wrote: > struct sst_pdata is equipped with fw_name field - a platform specific > filename for basefw module. Usage of such allows for suther > simplification of declaration of handlers directly involved with Skylake > initialization procedure. > > This change invalidates mach::fw_filename field and skl::fw_name. Again bad move. While in theory it's true that a single firmware is all you need, you do want to keep the ability to quirk firmware names for specific cases. We've been there before, don't remove this capability please. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/common/sst-acpi.c | 5 ++--- > sound/soc/intel/common/sst-firmware.c | 1 + > sound/soc/intel/skylake/skl-messages.c | 2 +- > sound/soc/intel/skylake/skl-sst-dsp.h | 3 +-- > sound/soc/intel/skylake/skl-sst-utils.c | 4 +--- > sound/soc/intel/skylake/skl.c | 4 ---- > 6 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c > index 53ac23f05966..15f2b27e643f 100644 > --- a/sound/soc/intel/common/sst-acpi.c > +++ b/sound/soc/intel/common/sst-acpi.c > @@ -28,11 +28,10 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context) > struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); > struct sst_acpi_desc *desc = sst_acpi->desc; > struct sst_pdata *sst_pdata = desc->pdata; > - struct snd_soc_acpi_mach *mach = sst_acpi->mach; > > sst_pdata->fw = fw; > if (!fw) { > - dev_err(dev, "Cannot load firmware %s\n", mach->fw_filename); > + dev_err(dev, "Cannot load firmware %s\n", sst_pdata->fw_name); > return; > } > > @@ -119,7 +118,7 @@ int sst_acpi_probe(struct platform_device *pdev) > return PTR_ERR(sst_acpi->pdev_mach); > > /* continue SST probing after firmware is loaded */ > - ret = request_firmware_nowait(THIS_MODULE, true, mach->fw_filename, > + ret = request_firmware_nowait(THIS_MODULE, true, sst_pdata->fw_name, > dev, GFP_KERNEL, pdev, sst_acpi_fw_cb); > if (ret) > platform_device_unregister(sst_acpi->pdev_mach); > diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c > index 61d3e6e46b98..cc88849eb10f 100644 > --- a/sound/soc/intel/common/sst-firmware.c > +++ b/sound/soc/intel/common/sst-firmware.c > @@ -1218,6 +1218,7 @@ struct sst_dsp *sst_dsp_new(struct device *dev, struct sst_pdata *pdata) > sst->thread_context = pdata->dsp; > sst->id = pdata->id; > sst->irq = pdata->irq; > + sst->fw_name = pdata->fw_name; > sst->ops = pdata->ops; > sst->pdata = pdata; > INIT_LIST_HEAD(&sst->used_block_list); > diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c > index 372c5fb83ddb..e401edd8d44b 100644 > --- a/sound/soc/intel/skylake/skl-messages.c > +++ b/sound/soc/intel/skylake/skl-messages.c > @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata) > if (!ops) > return -EIO; > > - ret = skl_sst_ctx_init(skl, skl->fw_name, pdata); > + ret = skl_sst_ctx_init(skl, pdata); > if (ret < 0) > return ret; > > diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h > index 8483c60f29ba..a3714b706b8e 100644 > --- a/sound/soc/intel/skylake/skl-sst-dsp.h > +++ b/sound/soc/intel/skylake/skl-sst-dsp.h > @@ -223,8 +223,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw); > > void skl_dsp_set_astate_cfg(struct skl_dev *skl, u32 cnt, void *data); > > -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, > - struct sst_pdata *pdata); > +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata); > int skl_prepare_lib_load(struct skl_dev *skl, struct skl_lib_info *linfo, > struct firmware *stripped_fw, > unsigned int hdr_offset, int index); > diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c > index a4ad213d34e0..ea5419012312 100644 > --- a/sound/soc/intel/skylake/skl-sst-utils.c > +++ b/sound/soc/intel/skylake/skl-sst-utils.c > @@ -354,8 +354,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw) > return 0; > } > > -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, > - struct sst_pdata *pdata) > +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata) > { > struct sst_dsp *sst; > struct device *dev = skl->dev; > @@ -372,7 +371,6 @@ int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, > } > > skl->dsp = sst; > - sst->fw_name = fw_name; > init_waitqueue_head(&skl->mod_load_wait); > skl->is_first_boot = true; > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index 39442c80a179..3225f4f8793e 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -491,9 +491,6 @@ static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl, > /* point to common table */ > mach = snd_soc_acpi_intel_hda_machines; > > - /* all entries in the machine table use the same firmware */ > - mach->fw_filename = machines->fw_filename; > - > return mach; > } > > @@ -514,7 +511,6 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) > } > > skl->mach = mach; > - skl->fw_name = mach->fw_filename; > pdata = mach->pdata; > > if (pdata) { >
On 2019-08-23 22:20, Pierre-Louis Bossart wrote: > > > On 8/22/19 2:04 PM, Cezary Rojewski wrote: >> struct sst_pdata is equipped with fw_name field - a platform specific >> filename for basefw module. Usage of such allows for suther >> simplification of declaration of handlers directly involved with Skylake >> initialization procedure. >> >> This change invalidates mach::fw_filename field and skl::fw_name. > > Again bad move. While in theory it's true that a single firmware is all > you need, you do want to keep the ability to quirk firmware names for > specific cases. We've been there before, don't remove this capability > please. > Explained this on PATCH 27/35. This is the task for topology manifest. Please note basefw binary alone does not cut it. There are external libraries and vendor cases too. In actual real-life specific example, you need to replace everything, e.g.: basefw, ARSC, WoV and so on. Don't see how fw_filename can represent several files, really. >> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/intel/common/sst-acpi.c | 5 ++--- >> sound/soc/intel/common/sst-firmware.c | 1 + >> sound/soc/intel/skylake/skl-messages.c | 2 +- >> sound/soc/intel/skylake/skl-sst-dsp.h | 3 +-- >> sound/soc/intel/skylake/skl-sst-utils.c | 4 +--- >> sound/soc/intel/skylake/skl.c | 4 ---- >> 6 files changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/sound/soc/intel/common/sst-acpi.c >> b/sound/soc/intel/common/sst-acpi.c >> index 53ac23f05966..15f2b27e643f 100644 >> --- a/sound/soc/intel/common/sst-acpi.c >> +++ b/sound/soc/intel/common/sst-acpi.c >> @@ -28,11 +28,10 @@ static void sst_acpi_fw_cb(const struct firmware >> *fw, void *context) >> struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); >> struct sst_acpi_desc *desc = sst_acpi->desc; >> struct sst_pdata *sst_pdata = desc->pdata; >> - struct snd_soc_acpi_mach *mach = sst_acpi->mach; >> sst_pdata->fw = fw; >> if (!fw) { >> - dev_err(dev, "Cannot load firmware %s\n", mach->fw_filename); >> + dev_err(dev, "Cannot load firmware %s\n", sst_pdata->fw_name); >> return; >> } >> @@ -119,7 +118,7 @@ int sst_acpi_probe(struct platform_device *pdev) >> return PTR_ERR(sst_acpi->pdev_mach); >> /* continue SST probing after firmware is loaded */ >> - ret = request_firmware_nowait(THIS_MODULE, true, mach->fw_filename, >> + ret = request_firmware_nowait(THIS_MODULE, true, sst_pdata->fw_name, >> dev, GFP_KERNEL, pdev, sst_acpi_fw_cb); >> if (ret) >> platform_device_unregister(sst_acpi->pdev_mach); >> diff --git a/sound/soc/intel/common/sst-firmware.c >> b/sound/soc/intel/common/sst-firmware.c >> index 61d3e6e46b98..cc88849eb10f 100644 >> --- a/sound/soc/intel/common/sst-firmware.c >> +++ b/sound/soc/intel/common/sst-firmware.c >> @@ -1218,6 +1218,7 @@ struct sst_dsp *sst_dsp_new(struct device *dev, >> struct sst_pdata *pdata) >> sst->thread_context = pdata->dsp; >> sst->id = pdata->id; >> sst->irq = pdata->irq; >> + sst->fw_name = pdata->fw_name; >> sst->ops = pdata->ops; >> sst->pdata = pdata; >> INIT_LIST_HEAD(&sst->used_block_list); >> diff --git a/sound/soc/intel/skylake/skl-messages.c >> b/sound/soc/intel/skylake/skl-messages.c >> index 372c5fb83ddb..e401edd8d44b 100644 >> --- a/sound/soc/intel/skylake/skl-messages.c >> +++ b/sound/soc/intel/skylake/skl-messages.c >> @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl, struct >> sst_pdata *pdata) >> if (!ops) >> return -EIO; >> - ret = skl_sst_ctx_init(skl, skl->fw_name, pdata); >> + ret = skl_sst_ctx_init(skl, pdata); >> if (ret < 0) >> return ret; >> diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h >> b/sound/soc/intel/skylake/skl-sst-dsp.h >> index 8483c60f29ba..a3714b706b8e 100644 >> --- a/sound/soc/intel/skylake/skl-sst-dsp.h >> +++ b/sound/soc/intel/skylake/skl-sst-dsp.h >> @@ -223,8 +223,7 @@ int skl_dsp_strip_extended_manifest(struct >> firmware *fw); >> void skl_dsp_set_astate_cfg(struct skl_dev *skl, u32 cnt, void *data); >> -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, >> - struct sst_pdata *pdata); >> +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata); >> int skl_prepare_lib_load(struct skl_dev *skl, struct skl_lib_info >> *linfo, >> struct firmware *stripped_fw, >> unsigned int hdr_offset, int index); >> diff --git a/sound/soc/intel/skylake/skl-sst-utils.c >> b/sound/soc/intel/skylake/skl-sst-utils.c >> index a4ad213d34e0..ea5419012312 100644 >> --- a/sound/soc/intel/skylake/skl-sst-utils.c >> +++ b/sound/soc/intel/skylake/skl-sst-utils.c >> @@ -354,8 +354,7 @@ int skl_dsp_strip_extended_manifest(struct >> firmware *fw) >> return 0; >> } >> -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, >> - struct sst_pdata *pdata) >> +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata) >> { >> struct sst_dsp *sst; >> struct device *dev = skl->dev; >> @@ -372,7 +371,6 @@ int skl_sst_ctx_init(struct skl_dev *skl, const >> char *fw_name, >> } >> skl->dsp = sst; >> - sst->fw_name = fw_name; >> init_waitqueue_head(&skl->mod_load_wait); >> skl->is_first_boot = true; >> diff --git a/sound/soc/intel/skylake/skl.c >> b/sound/soc/intel/skylake/skl.c >> index 39442c80a179..3225f4f8793e 100644 >> --- a/sound/soc/intel/skylake/skl.c >> +++ b/sound/soc/intel/skylake/skl.c >> @@ -491,9 +491,6 @@ static struct snd_soc_acpi_mach >> *skl_find_hda_machine(struct skl_dev *skl, >> /* point to common table */ >> mach = snd_soc_acpi_intel_hda_machines; >> - /* all entries in the machine table use the same firmware */ >> - mach->fw_filename = machines->fw_filename; >> - >> return mach; >> } >> @@ -514,7 +511,6 @@ static int skl_find_machine(struct skl_dev *skl, >> void *driver_data) >> } >> skl->mach = mach; >> - skl->fw_name = mach->fw_filename; >> pdata = mach->pdata; >> if (pdata) { >>
diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index 53ac23f05966..15f2b27e643f 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -28,11 +28,10 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context) struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); struct sst_acpi_desc *desc = sst_acpi->desc; struct sst_pdata *sst_pdata = desc->pdata; - struct snd_soc_acpi_mach *mach = sst_acpi->mach; sst_pdata->fw = fw; if (!fw) { - dev_err(dev, "Cannot load firmware %s\n", mach->fw_filename); + dev_err(dev, "Cannot load firmware %s\n", sst_pdata->fw_name); return; } @@ -119,7 +118,7 @@ int sst_acpi_probe(struct platform_device *pdev) return PTR_ERR(sst_acpi->pdev_mach); /* continue SST probing after firmware is loaded */ - ret = request_firmware_nowait(THIS_MODULE, true, mach->fw_filename, + ret = request_firmware_nowait(THIS_MODULE, true, sst_pdata->fw_name, dev, GFP_KERNEL, pdev, sst_acpi_fw_cb); if (ret) platform_device_unregister(sst_acpi->pdev_mach); diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 61d3e6e46b98..cc88849eb10f 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -1218,6 +1218,7 @@ struct sst_dsp *sst_dsp_new(struct device *dev, struct sst_pdata *pdata) sst->thread_context = pdata->dsp; sst->id = pdata->id; sst->irq = pdata->irq; + sst->fw_name = pdata->fw_name; sst->ops = pdata->ops; sst->pdata = pdata; INIT_LIST_HEAD(&sst->used_block_list); diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 372c5fb83ddb..e401edd8d44b 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata) if (!ops) return -EIO; - ret = skl_sst_ctx_init(skl, skl->fw_name, pdata); + ret = skl_sst_ctx_init(skl, pdata); if (ret < 0) return ret; diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 8483c60f29ba..a3714b706b8e 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -223,8 +223,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw); void skl_dsp_set_astate_cfg(struct skl_dev *skl, u32 cnt, void *data); -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, - struct sst_pdata *pdata); +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata); int skl_prepare_lib_load(struct skl_dev *skl, struct skl_lib_info *linfo, struct firmware *stripped_fw, unsigned int hdr_offset, int index); diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c index a4ad213d34e0..ea5419012312 100644 --- a/sound/soc/intel/skylake/skl-sst-utils.c +++ b/sound/soc/intel/skylake/skl-sst-utils.c @@ -354,8 +354,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw) return 0; } -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, - struct sst_pdata *pdata) +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata) { struct sst_dsp *sst; struct device *dev = skl->dev; @@ -372,7 +371,6 @@ int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, } skl->dsp = sst; - sst->fw_name = fw_name; init_waitqueue_head(&skl->mod_load_wait); skl->is_first_boot = true; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 39442c80a179..3225f4f8793e 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -491,9 +491,6 @@ static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl, /* point to common table */ mach = snd_soc_acpi_intel_hda_machines; - /* all entries in the machine table use the same firmware */ - mach->fw_filename = machines->fw_filename; - return mach; } @@ -514,7 +511,6 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) } skl->mach = mach; - skl->fw_name = mach->fw_filename; pdata = mach->pdata; if (pdata) {
struct sst_pdata is equipped with fw_name field - a platform specific filename for basefw module. Usage of such allows for suther simplification of declaration of handlers directly involved with Skylake initialization procedure. This change invalidates mach::fw_filename field and skl::fw_name. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/common/sst-acpi.c | 5 ++--- sound/soc/intel/common/sst-firmware.c | 1 + sound/soc/intel/skylake/skl-messages.c | 2 +- sound/soc/intel/skylake/skl-sst-dsp.h | 3 +-- sound/soc/intel/skylake/skl-sst-utils.c | 4 +--- sound/soc/intel/skylake/skl.c | 4 ---- 6 files changed, 6 insertions(+), 13 deletions(-)