Re: [PATCH 2/2] ASoC: Intel: sst-acpi: Request firmware before SST platform driver probing

[Takashi Iwai <tiwai@suse.de>]

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 <jarkko.nikula@linux.intel.com>
> Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> ---
>  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 <linux/acpi.h>
>  #include <linux/device.h>
> +#include <linux/firmware.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  
> @@ -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
>