From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 2/2] ASoC: Intel: sst-acpi: Request firmware before SST platform driver probing Date: Tue, 18 Feb 2014 15:58:12 +0100 Message-ID: References: <1392734523-5109-1-git-send-email-jarkko.nikula@linux.intel.com> <1392734523-5109-2-git-send-email-jarkko.nikula@linux.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id DF8892651D8 for ; Tue, 18 Feb 2014 15:58:12 +0100 (CET) In-Reply-To: <1392734523-5109-2-git-send-email-jarkko.nikula@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Jarkko Nikula Cc: Liam Girdwood , alsa-devel@alsa-project.org, Mark Brown , Liam Girdwood List-Id: alsa-devel@alsa-project.org At Tue, 18 Feb 2014 16:42:03 +0200, Jarkko Nikula wrote: > > We originally thought to request SST audio DSP firmware during the SST > platform driver initialization. However plain request_firmware doesn't > work in driver probe paths if userspace is not ready to handle it. For > instance when drivers are built-in. > > Implementing asynchronous firmware request in SST platform driver > initialization complicates code needlessly since it anyway will fail if > firmware is missing. > > This is more simple to handle by requesting firmware asynchronously in > sst_acpi_probe() and register SST platform only after firmware is loaded. > > Signed-off-by: Jarkko Nikula > Acked-by: Liam Girdwood > --- > sound/soc/intel/sst-acpi.c | 54 ++++++++++++++++++++++++++++++---------------- > sound/soc/intel/sst-dsp.h | 2 +- > 2 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/sound/soc/intel/sst-acpi.c b/sound/soc/intel/sst-acpi.c > index aba73ca8a923..b76d9d9f15c0 100644 > --- a/sound/soc/intel/sst-acpi.c > +++ b/sound/soc/intel/sst-acpi.c > @@ -16,6 +16,7 @@ > > #include > #include > +#include > #include > #include > > @@ -56,6 +57,32 @@ struct sst_acpi_priv { > struct sst_acpi_desc *desc; > }; > > +static void sst_acpi_fw_cb(const struct firmware *fw, void *context) > +{ > + struct platform_device *pdev = context; > + struct device *dev = &pdev->dev; > + struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); > + struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata; > + struct sst_acpi_desc *desc = sst_acpi->desc; > + > + sst_pdata->fw = fw; > + if (!fw) { > + dev_err(dev, "Cannot load firmware %s\n", desc->fw_filename); > + return; > + } > + > + /* register PCM and DAI driver */ > + sst_acpi->pdev_pcm = > + platform_device_register_data(dev, desc->drv_name, -1, > + sst_pdata, sizeof(*sst_pdata)); > + if (IS_ERR(sst_acpi->pdev_pcm)) { > + dev_err(dev, "Cannot register device %s. Error %d\n", > + desc->drv_name, (int)PTR_ERR(sst_acpi->pdev_pcm)); > + } > + > + return; > +} > + > static int sst_acpi_probe(struct platform_device *pdev) > { > const struct acpi_device_id *id; > @@ -65,7 +92,6 @@ static int sst_acpi_probe(struct platform_device *pdev) > struct sst_acpi_mach *mach; > struct sst_acpi_desc *desc; > struct resource *mmio; > - int ret = 0; > > sst_acpi = devm_kzalloc(dev, sizeof(*sst_acpi), GFP_KERNEL); > if (sst_acpi == NULL) > @@ -79,7 +105,6 @@ static int sst_acpi_probe(struct platform_device *pdev) > desc = mach->res_desc; > sst_pdata = &sst_acpi->sst_pdata; > sst_pdata->id = desc->sst_id; > - sst_pdata->fw_filename = desc->fw_filename; > sst_acpi->desc = desc; > > if (desc->resindex_dma_base >= 0) { > @@ -118,37 +143,28 @@ static int sst_acpi_probe(struct platform_device *pdev) > } > } > > - /* register PCM and DAI driver */ > - sst_acpi->pdev_pcm = > - platform_device_register_data(dev, desc->drv_name, -1, > - sst_pdata, sizeof(*sst_pdata)); > - if (IS_ERR(sst_acpi->pdev_pcm)) > - return PTR_ERR(sst_acpi->pdev_pcm); > - > - /* register machine driver */ > platform_set_drvdata(pdev, sst_acpi); > > + /* register machine driver */ > sst_acpi->pdev_mach = > platform_device_register_data(dev, mach->drv_name, -1, > sst_pdata, sizeof(*sst_pdata)); > - if (IS_ERR(sst_acpi->pdev_mach)) { > - ret = PTR_ERR(sst_acpi->pdev_mach); > - goto sst_err; > - } > - > - return ret; > + if (IS_ERR(sst_acpi->pdev_mach)) > + return PTR_ERR(sst_acpi->pdev_mach); > > -sst_err: > - platform_device_unregister(sst_acpi->pdev_pcm); > - return ret; > + /* continue SST probing after firmware is loaded */ > + return request_firmware_nowait(THIS_MODULE, true, desc->fw_filename, > + dev, GFP_KERNEL, pdev, sst_acpi_fw_cb); sst_acpi->pdev_mach still should be unregistered when request_firmware_nowait() returns an error. > } > > static int sst_acpi_remove(struct platform_device *pdev) > { > struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); > + struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata; > > platform_device_unregister(sst_acpi->pdev_mach); > platform_device_unregister(sst_acpi->pdev_pcm); With your patch, pdev_pcm isn't always a valid pointer. You can't pass it unconditionally any longer. Takashi > + release_firmware(sst_pdata->fw); > > return 0; > } > diff --git a/sound/soc/intel/sst-dsp.h b/sound/soc/intel/sst-dsp.h > index d134359fecac..3730fd324455 100644 > --- a/sound/soc/intel/sst-dsp.h > +++ b/sound/soc/intel/sst-dsp.h > @@ -152,7 +152,7 @@ struct sst_pdata { > int irq; > > /* Firmware */ > - const char *fw_filename; > + const struct firmware *fw; > > /* DMA */ > u32 dma_base; > -- > 1.8.5.3 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >