Linux Sound subsystem development
 help / color / mirror / Atom feed
* [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres
@ 2025-04-23  8:28 Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 1/4] ASoC: sof: Use pure devres PCI Philipp Stanner
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-04-23  8:28 UTC (permalink / raw)
  To: Cezary Rojewski, Liam Girdwood, Peter Ujfalusi, Bard Liao,
	Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Daniel Baluta, Philipp Stanner,
	Amadeusz Sławiński, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

Changes in v2:
  - sof: simplify return. (Andy)
  - intel/atom: simplify return. (Andy)
  - Send a separate series for AsoC. (Andy)
  - intel/atom: Add another patch that switches EINVAL to ENOMEM. (Andy)

Hi,

a year ago we spent quite some work trying to get PCI into better shape.
Some pci_ functions can be sometimes managed with devres, which is
obviously bad. We want to provide an obvious API, where pci_ functions
are never, and pcim_ functions are always managed.

Thus, everyone enabling his device with pcim_enable_device() must be
ported to pcim_ functions. Porting all users will later enable us to
significantly simplify parts of the PCI subsystem. See here [1] for
details.

This patch series does that for sound.

Feel free to squash the commits as you see fit.

P.

[1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/pci/devres.c#L18

Philipp Stanner (4):
  ASoC: sof: Use pure devres PCI
  ASoC: intel/avs: Use pure devres PCI
  AsoC: intel/atom: Use pure devres PCI
  AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails

 sound/soc/intel/atom/sst/sst_pci.c | 58 +++++++++++++-----------------
 sound/soc/intel/avs/core.c         |  7 ++--
 sound/soc/sof/sof-pci-dev.c        | 16 ++-------
 3 files changed, 29 insertions(+), 52 deletions(-)

-- 
2.48.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] ASoC: sof: Use pure devres PCI
  2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
@ 2025-04-23  8:28 ` Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 2/4] ASoC: intel/avs: " Philipp Stanner
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-04-23  8:28 UTC (permalink / raw)
  To: Cezary Rojewski, Liam Girdwood, Peter Ujfalusi, Bard Liao,
	Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Daniel Baluta, Philipp Stanner,
	Amadeusz Sławiński, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

pci_request_regions() is a hybrid function which becomes managed if
pcim_enable_device() was called before. This hybrid nature is deprecated
and should not be used anymore.

Replace pci_request_regions() with the always-managed function
pcim_request_all_regions().

Remove surplus calls to PCI release functions, since pcim_ functions do
cleanup automatically.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 sound/soc/sof/sof-pci-dev.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
index 2fc14b9a33d4..c50249aadea9 100644
--- a/sound/soc/sof/sof-pci-dev.c
+++ b/sound/soc/sof/sof-pci-dev.c
@@ -216,7 +216,7 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
 	if (ret < 0)
 		return ret;
 
-	ret = pci_request_regions(pci, "Audio DSP");
+	ret = pcim_request_all_regions(pci, "Audio DSP");
 	if (ret < 0)
 		return ret;
 
@@ -240,8 +240,7 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
 		path_override->ipc_type = sof_pci_ipc_type;
 	} else {
 		dev_err(dev, "Invalid IPC type requested: %d\n", sof_pci_ipc_type);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	path_override->fw_path = fw_path;
@@ -271,13 +270,7 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
 	sof_pdata->sof_probe_complete = sof_pci_probe_complete;
 
 	/* call sof helper for DSP hardware probe */
-	ret = snd_sof_device_probe(dev, sof_pdata);
-
-out:
-	if (ret)
-		pci_release_regions(pci);
-
-	return ret;
+	return snd_sof_device_probe(dev, sof_pdata);
 }
 EXPORT_SYMBOL_NS(sof_pci_probe, "SND_SOC_SOF_PCI_DEV");
 
@@ -290,9 +283,6 @@ void sof_pci_remove(struct pci_dev *pci)
 	if (snd_sof_device_probe_completed(&pci->dev) &&
 	    !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME))
 		pm_runtime_get_noresume(&pci->dev);
-
-	/* release pci regions and disable device */
-	pci_release_regions(pci);
 }
 EXPORT_SYMBOL_NS(sof_pci_remove, "SND_SOC_SOF_PCI_DEV");
 
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] ASoC: intel/avs: Use pure devres PCI
  2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 1/4] ASoC: sof: Use pure devres PCI Philipp Stanner
@ 2025-04-23  8:28 ` Philipp Stanner
  2025-04-24 10:21   ` Amadeusz Sławiński
  2025-04-23  8:28 ` [PATCH v2 3/4] AsoC: intel/atom: " Philipp Stanner
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Philipp Stanner @ 2025-04-23  8:28 UTC (permalink / raw)
  To: Cezary Rojewski, Liam Girdwood, Peter Ujfalusi, Bard Liao,
	Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Daniel Baluta, Philipp Stanner,
	Amadeusz Sławiński, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

pci_request_regions() is a hybrid function which becomes managed if
pcim_enable_device() was called before. This hybrid nature is deprecated
and should not be used anymore.

Replace pci_request_regions() with the always-managed function
pcim_request_all_regions().

Remove the goto jump to pci_release_regions(), since pcim_ functions
clean up automatically.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 sound/soc/intel/avs/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 8fbf33e30dfc..dafe46973146 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -445,7 +445,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return ret;
 	}
 
-	ret = pci_request_regions(pci, "AVS HDAudio");
+	ret = pcim_request_all_regions(pci, "AVS HDAudio");
 	if (ret < 0)
 		return ret;
 
@@ -454,8 +454,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	bus->remap_addr = pci_ioremap_bar(pci, 0);
 	if (!bus->remap_addr) {
 		dev_err(bus->dev, "ioremap error\n");
-		ret = -ENXIO;
-		goto err_remap_bar0;
+		return -ENXIO;
 	}
 
 	adev->dsp_ba = pci_ioremap_bar(pci, 4);
@@ -512,8 +511,6 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	iounmap(adev->dsp_ba);
 err_remap_bar4:
 	iounmap(bus->remap_addr);
-err_remap_bar0:
-	pci_release_regions(pci);
 	return ret;
 }
 
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] AsoC: intel/atom: Use pure devres PCI
  2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 1/4] ASoC: sof: Use pure devres PCI Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 2/4] ASoC: intel/avs: " Philipp Stanner
@ 2025-04-23  8:28 ` Philipp Stanner
  2025-04-23  8:28 ` [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails Philipp Stanner
  2025-04-23 12:33 ` [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Cezary Rojewski
  4 siblings, 0 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-04-23  8:28 UTC (permalink / raw)
  To: Cezary Rojewski, Liam Girdwood, Peter Ujfalusi, Bard Liao,
	Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Daniel Baluta, Philipp Stanner,
	Amadeusz Sławiński, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

pci_request_regions() is a hybrid function which becomes managed if
pcim_enable_device() was called before. This hybrid nature is deprecated
and should not be used anymore.

Replace pci_request_regions() with the always-managed function
pcim_request_all_regions().

Remove the call to pci_release_regions(), since pcim_ functions do
cleanup automatically.

Pass 0 as length parameter to pcim_iomap(), which is the standard way
for ioremapping an entire BAR.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 sound/soc/intel/atom/sst/sst_pci.c | 58 +++++++++++++-----------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c
index d1e64c3500be..eadcf24cbdc3 100644
--- a/sound/soc/intel/atom/sst/sst_pci.c
+++ b/sound/soc/intel/atom/sst/sst_pci.c
@@ -26,7 +26,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	int ddr_base, ret = 0;
 	struct pci_dev *pci = ctx->pci;
 
-	ret = pci_request_regions(pci, SST_DRV_NAME);
+	ret = pcim_request_all_regions(pci, SST_DRV_NAME);
 	if (ret)
 		return ret;
 
@@ -38,67 +38,57 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 		ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base);
 		if (!ctx->pdata->lib_info) {
 			dev_err(ctx->dev, "lib_info pointer NULL\n");
-			ret = -EINVAL;
-			goto do_release_regions;
+			return -EINVAL;
 		}
 		if (ddr_base != ctx->pdata->lib_info->mod_base) {
 			dev_err(ctx->dev,
 					"FW LSP DDR BASE does not match with IFWI\n");
-			ret = -EINVAL;
-			goto do_release_regions;
+			return -EINVAL;
 		}
 		ctx->ddr_end = pci_resource_end(pci, 0);
 
-		ctx->ddr = pcim_iomap(pci, 0,
-					pci_resource_len(pci, 0));
-		if (!ctx->ddr) {
-			ret = -EINVAL;
-			goto do_release_regions;
-		}
+		ctx->ddr = pcim_iomap(pci, 0, 0);
+		if (!ctx->ddr)
+			return -EINVAL;
+
 		dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
 	} else {
 		ctx->ddr = NULL;
 	}
 	/* SHIM */
 	ctx->shim_phy_add = pci_resource_start(pci, 1);
-	ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1));
-	if (!ctx->shim) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
+	ctx->shim = pcim_iomap(pci, 1, 0);
+	if (!ctx->shim)
+		return -EINVAL;
+
 	dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
 
 	/* Shared SRAM */
 	ctx->mailbox_add = pci_resource_start(pci, 2);
-	ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2));
-	if (!ctx->mailbox) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
+	ctx->mailbox = pcim_iomap(pci, 2, 0);
+	if (!ctx->mailbox)
+		return -EINVAL;
+
 	dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
 
 	/* IRAM */
 	ctx->iram_end = pci_resource_end(pci, 3);
 	ctx->iram_base = pci_resource_start(pci, 3);
-	ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3));
-	if (!ctx->iram) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
+	ctx->iram = pcim_iomap(pci, 3, 0);
+	if (!ctx->iram)
+		return -EINVAL;
+
 	dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
 
 	/* DRAM */
 	ctx->dram_end = pci_resource_end(pci, 4);
 	ctx->dram_base = pci_resource_start(pci, 4);
-	ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4));
-	if (!ctx->dram) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
+	ctx->dram = pcim_iomap(pci, 4, 0);
+	if (!ctx->dram)
+		return -EINVAL;
+
 	dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
-do_release_regions:
-	pci_release_regions(pci);
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails
  2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
                   ` (2 preceding siblings ...)
  2025-04-23  8:28 ` [PATCH v2 3/4] AsoC: intel/atom: " Philipp Stanner
@ 2025-04-23  8:28 ` Philipp Stanner
  2025-04-23 12:28   ` Cezary Rojewski
  2025-04-23 12:33 ` [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Cezary Rojewski
  4 siblings, 1 reply; 9+ messages in thread
From: Philipp Stanner @ 2025-04-23  8:28 UTC (permalink / raw)
  To: Cezary Rojewski, Liam Girdwood, Peter Ujfalusi, Bard Liao,
	Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Daniel Baluta, Philipp Stanner,
	Amadeusz Sławiński, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

The error checks for pcim_iomap() have the function return -EINVAL.
-ENOMEM is a more appropriate error code.

Replace -EINVAL with -ENOMEM.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 sound/soc/intel/atom/sst/sst_pci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c
index eadcf24cbdc3..edc86519816d 100644
--- a/sound/soc/intel/atom/sst/sst_pci.c
+++ b/sound/soc/intel/atom/sst/sst_pci.c
@@ -49,7 +49,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 
 		ctx->ddr = pcim_iomap(pci, 0, 0);
 		if (!ctx->ddr)
-			return -EINVAL;
+			return -ENOMEM;
 
 		dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
 	} else {
@@ -59,7 +59,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	ctx->shim_phy_add = pci_resource_start(pci, 1);
 	ctx->shim = pcim_iomap(pci, 1, 0);
 	if (!ctx->shim)
-		return -EINVAL;
+		return -ENOMEM;
 
 	dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
 
@@ -67,7 +67,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	ctx->mailbox_add = pci_resource_start(pci, 2);
 	ctx->mailbox = pcim_iomap(pci, 2, 0);
 	if (!ctx->mailbox)
-		return -EINVAL;
+		return -ENOMEM;
 
 	dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
 
@@ -76,7 +76,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	ctx->iram_base = pci_resource_start(pci, 3);
 	ctx->iram = pcim_iomap(pci, 3, 0);
 	if (!ctx->iram)
-		return -EINVAL;
+		return -ENOMEM;
 
 	dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
 
@@ -85,7 +85,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	ctx->dram_base = pci_resource_start(pci, 4);
 	ctx->dram = pcim_iomap(pci, 4, 0);
 	if (!ctx->dram)
-		return -EINVAL;
+		return -ENOMEM;
 
 	dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
 	return 0;
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails
  2025-04-23  8:28 ` [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails Philipp Stanner
@ 2025-04-23 12:28   ` Cezary Rojewski
  0 siblings, 0 replies; 9+ messages in thread
From: Cezary Rojewski @ 2025-04-23 12:28 UTC (permalink / raw)
  To: Philipp Stanner
  Cc: linux-sound, linux-kernel, sound-open-firmware, Liam Girdwood,
	Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Kai Vehmanen,
	Pierre-Louis Bossart, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Daniel Baluta, Amadeusz Sławiński, Charles Keepax,
	Damien Le Moal

On 2025-04-23 10:28 AM, Philipp Stanner wrote:
> The error checks for pcim_iomap() have the function return -EINVAL.
> -ENOMEM is a more appropriate error code.
> 
> Replace -EINVAL with -ENOMEM.
Nitpicks:

I believe the last sentence is redundant, the title and the message say 
it all.

Next, the suggest scope for the atom-driver is: 'ASoC: Intel: atom:'.

> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   sound/soc/intel/atom/sst/sst_pci.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c
> index eadcf24cbdc3..edc86519816d 100644
> --- a/sound/soc/intel/atom/sst/sst_pci.c
> +++ b/sound/soc/intel/atom/sst/sst_pci.c
> @@ -49,7 +49,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   
>   		ctx->ddr = pcim_iomap(pci, 0, 0);
>   		if (!ctx->ddr)
> -			return -EINVAL;
> +			return -ENOMEM;
>   
>   		dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
>   	} else {
> @@ -59,7 +59,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   	ctx->shim_phy_add = pci_resource_start(pci, 1);
>   	ctx->shim = pcim_iomap(pci, 1, 0);
>   	if (!ctx->shim)
> -		return -EINVAL;
> +		return -ENOMEM;
>   
>   	dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
>   
> @@ -67,7 +67,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   	ctx->mailbox_add = pci_resource_start(pci, 2);
>   	ctx->mailbox = pcim_iomap(pci, 2, 0);
>   	if (!ctx->mailbox)
> -		return -EINVAL;
> +		return -ENOMEM;
>   
>   	dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
>   
> @@ -76,7 +76,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   	ctx->iram_base = pci_resource_start(pci, 3);
>   	ctx->iram = pcim_iomap(pci, 3, 0);
>   	if (!ctx->iram)
> -		return -EINVAL;
> +		return -ENOMEM;
>   
>   	dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
>   
> @@ -85,7 +85,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   	ctx->dram_base = pci_resource_start(pci, 4);
>   	ctx->dram = pcim_iomap(pci, 4, 0);
>   	if (!ctx->dram)
> -		return -EINVAL;
> +		return -ENOMEM;
>   
>   	dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
>   	return 0;


Hi Philipp,

Thanks for the patch, this is certainly an additional effort, on top of 
the pcim_xxx one. Couple of nitpicks above but nothing major. Regardless 
if you decide to address them or not, feel free to add:

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres
  2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
                   ` (3 preceding siblings ...)
  2025-04-23  8:28 ` [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails Philipp Stanner
@ 2025-04-23 12:33 ` Cezary Rojewski
  4 siblings, 0 replies; 9+ messages in thread
From: Cezary Rojewski @ 2025-04-23 12:33 UTC (permalink / raw)
  To: Philipp Stanner
  Cc: linux-sound, linux-kernel, sound-open-firmware, Liam Girdwood,
	Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Kai Vehmanen,
	Pierre-Louis Bossart, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Daniel Baluta, Amadeusz Sławiński, Charles Keepax,
	Damien Le Moal

On 2025-04-23 10:28 AM, Philipp Stanner wrote:
> Changes in v2:
>    - sof: simplify return. (Andy)
>    - intel/atom: simplify return. (Andy)
>    - Send a separate series for AsoC. (Andy)
>    - intel/atom: Add another patch that switches EINVAL to ENOMEM. (Andy)
> 
> Hi,
> 
> a year ago we spent quite some work trying to get PCI into better shape.
> Some pci_ functions can be sometimes managed with devres, which is
> obviously bad. We want to provide an obvious API, where pci_ functions
> are never, and pcim_ functions are always managed.
> 
> Thus, everyone enabling his device with pcim_enable_device() must be
> ported to pcim_ functions. Porting all users will later enable us to
> significantly simplify parts of the PCI subsystem. See here [1] for
> details.
> 
> This patch series does that for sound.
> 
> Feel free to squash the commits as you see fit.
> 
> P.
> 
> [1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/pci/devres.c#L18
> 
> Philipp Stanner (4):
>    ASoC: sof: Use pure devres PCI
>    ASoC: intel/avs: Use pure devres PCI
>    AsoC: intel/atom: Use pure devres PCI
>    AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails

Nitpick: the scopes used in commit titles do not match recommendations. 
Below the suggested ones:
	'ASoC: Intel: atom:' for the atom-driver
	'ASoC: Intel: avs:' for the avs-driver

>   sound/soc/intel/atom/sst/sst_pci.c | 58 +++++++++++++-----------------
>   sound/soc/intel/avs/core.c         |  7 ++--
>   sound/soc/sof/sof-pci-dev.c        | 16 ++-------
>   3 files changed, 29 insertions(+), 52 deletions(-)
> 

Hi Philipp,

Thank you for the contribution. I do not see any major issues so feel 
free to add:

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

Kind Regards,
Czarek

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] ASoC: intel/avs: Use pure devres PCI
  2025-04-23  8:28 ` [PATCH v2 2/4] ASoC: intel/avs: " Philipp Stanner
@ 2025-04-24 10:21   ` Amadeusz Sławiński
  2025-04-24 11:33     ` Philipp Stanner
  0 siblings, 1 reply; 9+ messages in thread
From: Amadeusz Sławiński @ 2025-04-24 10:21 UTC (permalink / raw)
  To: Philipp Stanner, Cezary Rojewski, Liam Girdwood, Peter Ujfalusi,
	Bard Liao, Ranjani Sridharan, Kai Vehmanen, Pierre-Louis Bossart,
	Mark Brown, Jaroslav Kysela, Takashi Iwai, Daniel Baluta,
	Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware



On 2025-04-23 10:28, Philipp Stanner wrote:
> pci_request_regions() is a hybrid function which becomes managed if
> pcim_enable_device() was called before. This hybrid nature is deprecated
> and should not be used anymore.
> 
> Replace pci_request_regions() with the always-managed function
> pcim_request_all_regions().
> 
> Remove the goto jump to pci_release_regions(), since pcim_ functions
> clean up automatically.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   sound/soc/intel/avs/core.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> index 8fbf33e30dfc..dafe46973146 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c
> @@ -445,7 +445,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   		return ret;
>   	}
>   
> -	ret = pci_request_regions(pci, "AVS HDAudio");
> +	ret = pcim_request_all_regions(pci, "AVS HDAudio");
>   	if (ret < 0)
>   		return ret;
>   
> @@ -454,8 +454,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   	bus->remap_addr = pci_ioremap_bar(pci, 0);
>   	if (!bus->remap_addr) {
>   		dev_err(bus->dev, "ioremap error\n");
> -		ret = -ENXIO;
> -		goto err_remap_bar0;
> +		return -ENXIO;
>   	}
>   
>   	adev->dsp_ba = pci_ioremap_bar(pci, 4);
> @@ -512,8 +511,6 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   	iounmap(adev->dsp_ba);
>   err_remap_bar4:
>   	iounmap(bus->remap_addr);
> -err_remap_bar0:
> -	pci_release_regions(pci);

Hm... shouldn't we also drop call to pci_release_regions() in 
avs_pci_remove()?

>   	return ret;
>   }
>   

Nitpick: If there will be v2, can you also align title with how it 
usually is in this directory:
ASoC: Intel: avs: Use pure devres PCI


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] ASoC: intel/avs: Use pure devres PCI
  2025-04-24 10:21   ` Amadeusz Sławiński
@ 2025-04-24 11:33     ` Philipp Stanner
  0 siblings, 0 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-04-24 11:33 UTC (permalink / raw)
  To: Amadeusz Sławiński, Philipp Stanner, Cezary Rojewski,
	Liam Girdwood, Peter Ujfalusi, Bard Liao, Ranjani Sridharan,
	Kai Vehmanen, Pierre-Louis Bossart, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Daniel Baluta, Charles Keepax, Damien Le Moal
  Cc: linux-sound, linux-kernel, sound-open-firmware

On Thu, 2025-04-24 at 12:21 +0200, Amadeusz Sławiński wrote:
> 
> 
> On 2025-04-23 10:28, Philipp Stanner wrote:
> > pci_request_regions() is a hybrid function which becomes managed if
> > pcim_enable_device() was called before. This hybrid nature is
> > deprecated
> > and should not be used anymore.
> > 
> > Replace pci_request_regions() with the always-managed function
> > pcim_request_all_regions().
> > 
> > Remove the goto jump to pci_release_regions(), since pcim_
> > functions
> > clean up automatically.
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >   sound/soc/intel/avs/core.c | 7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/soc/intel/avs/core.c
> > b/sound/soc/intel/avs/core.c
> > index 8fbf33e30dfc..dafe46973146 100644
> > --- a/sound/soc/intel/avs/core.c
> > +++ b/sound/soc/intel/avs/core.c
> > @@ -445,7 +445,7 @@ static int avs_pci_probe(struct pci_dev *pci,
> > const struct pci_device_id *id)
> >   		return ret;
> >   	}
> >   
> > -	ret = pci_request_regions(pci, "AVS HDAudio");
> > +	ret = pcim_request_all_regions(pci, "AVS HDAudio");
> >   	if (ret < 0)
> >   		return ret;
> >   
> > @@ -454,8 +454,7 @@ static int avs_pci_probe(struct pci_dev *pci,
> > const struct pci_device_id *id)
> >   	bus->remap_addr = pci_ioremap_bar(pci, 0);
> >   	if (!bus->remap_addr) {
> >   		dev_err(bus->dev, "ioremap error\n");
> > -		ret = -ENXIO;
> > -		goto err_remap_bar0;
> > +		return -ENXIO;
> >   	}
> >   
> >   	adev->dsp_ba = pci_ioremap_bar(pci, 4);
> > @@ -512,8 +511,6 @@ static int avs_pci_probe(struct pci_dev *pci,
> > const struct pci_device_id *id)
> >   	iounmap(adev->dsp_ba);
> >   err_remap_bar4:
> >   	iounmap(bus->remap_addr);
> > -err_remap_bar0:
> > -	pci_release_regions(pci);
> 
> Hm... shouldn't we also drop call to pci_release_regions() in 
> avs_pci_remove()?

Oh, yes, we should!

And in soc/sof/sof-pci-dev.c it slipped me too.

Will reiterate.

Thx
P.

> 
> >   	return ret;
> >   }
> >   
> 
> Nitpick: If there will be v2, can you also align title with how it 
> usually is in this directory:
> ASoC: Intel: avs: Use pure devres PCI
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-04-24 11:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-23  8:28 [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Philipp Stanner
2025-04-23  8:28 ` [PATCH v2 1/4] ASoC: sof: Use pure devres PCI Philipp Stanner
2025-04-23  8:28 ` [PATCH v2 2/4] ASoC: intel/avs: " Philipp Stanner
2025-04-24 10:21   ` Amadeusz Sławiński
2025-04-24 11:33     ` Philipp Stanner
2025-04-23  8:28 ` [PATCH v2 3/4] AsoC: intel/atom: " Philipp Stanner
2025-04-23  8:28 ` [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails Philipp Stanner
2025-04-23 12:28   ` Cezary Rojewski
2025-04-23 12:33 ` [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres Cezary Rojewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox