[alsa-devel] [PATCH 1/2] ASoC: Intel: Move extended fw base and size fields in struct sst_pdata
Move fw_base and fw_size fields in struct sst_pdata under ACPI data for clarifying that these are not related to firmware file but for platform specific extended firmware area reserved by the BIOS.
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com Acked-by: Liam Girdwood liam.r.girdwood@linux.intel.com --- sound/soc/intel/sst-dsp.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/sst-dsp.h b/sound/soc/intel/sst-dsp.h index 0ce5c8d91794..d134359fecac 100644 --- a/sound/soc/intel/sst-dsp.h +++ b/sound/soc/intel/sst-dsp.h @@ -147,12 +147,12 @@ struct sst_pdata { u32 lpe_size; u32 pcicfg_base; u32 pcicfg_size; + u32 fw_base; + u32 fw_size; int irq;
/* Firmware */ const char *fw_filename; - u32 fw_base; - u32 fw_size;
/* DMA */ u32 dma_base;
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); }
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); + 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;
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
Hi
On 02/18/2014 04:58 PM, Takashi Iwai wrote:
At Tue, 18 Feb 2014 16:42:03 +0200, Jarkko Nikula wrote:
-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.
I was thinking to leave that for sst_acpi_remove but you are right, it doesn't make sense to leave it registered for instance if request_firmware_nowait fails because of -ENOMEM or some other fatal issue.
}
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.
I felt it was needless to test NULL pointers here since release_firmware checks it directly and platform_device_unregister indirectly. Not in platform_device_unregister but when calling platform_device_del and platform_device_put there.
On 02/19/2014 08:57 AM, Jarkko Nikula wrote:
Hi
On 02/18/2014 04:58 PM, Takashi Iwai wrote:
At Tue, 18 Feb 2014 16:42:03 +0200, Jarkko Nikula wrote:
-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.
I was thinking to leave that for sst_acpi_remove but you are right, it doesn't make sense to leave it registered for instance if request_firmware_nowait fails because of -ENOMEM or some other fatal issue.
Answering myself after cup of coffee... Obviously probe function must free whatever it allocated when it returns with an error. In other words module loading fails and we won't ever call the remove function.
At Wed, 19 Feb 2014 08:57:53 +0200, Jarkko Nikula wrote:
Hi
On 02/18/2014 04:58 PM, Takashi Iwai wrote:
At Tue, 18 Feb 2014 16:42:03 +0200, Jarkko Nikula wrote:
-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.
I was thinking to leave that for sst_acpi_remove but you are right, it doesn't make sense to leave it registered for instance if request_firmware_nowait fails because of -ENOMEM or some other fatal issue.
}
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.
I felt it was needless to test NULL pointers here since release_firmware checks it directly and platform_device_unregister indirectly. Not in platform_device_unregister but when calling platform_device_del and platform_device_put there.
The problem is that it may contain ERR_PTR(xxx). You have to either clear to NULL in
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)); sst_acpi->pdev_pcm = NULL; }
or check conditionally like
if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm)) platform_device_unregister(sst_acpi->pdev_pcm); Maybe the former is better.
Takashi
Hi
On 02/19/2014 09:15 AM, Takashi Iwai wrote:
At Wed, 19 Feb 2014 08:57:53 +0200, Jarkko Nikula wrote:
I felt it was needless to test NULL pointers here since release_firmware checks it directly and platform_device_unregister indirectly. Not in platform_device_unregister but when calling platform_device_del and platform_device_put there.
The problem is that it may contain ERR_PTR(xxx). You have to either clear to NULL in
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)); sst_acpi->pdev_pcm = NULL; }
or check conditionally like
if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm)) platform_device_unregister(sst_acpi->pdev_pcm);
Maybe the former is better.
Oh yes and I'm even testing it with the IS_ERR and PTR_ERR. Why that knowledge did not carry to sst_acpi_remove... will fix.
On Tue, Feb 18, 2014 at 04:42:02PM +0200, Jarkko Nikula wrote:
Move fw_base and fw_size fields in struct sst_pdata under ACPI data for clarifying that these are not related to firmware file but for platform specific extended firmware area reserved by the BIOS.
Applied, thanks.
participants (3)
-
Jarkko Nikula
-
Mark Brown
-
Takashi Iwai