All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.