Re: [PATCH] drm/ast: use managed devres functions

[Jocelyn Falempe]

On 30/10/2023 10:50, Philipp Stanner wrote:
Currently, tha ast-driver just maps the PCI-dev's regions with
pcim_iomap(). It does not actually reserve the regions exclusively
with, e.g., pci_request_regions().
Replace the calls to pcim_iomap() with ones to pcim_iomap_regions() to
reserve and map the regions simultaneously.

Suggested-by: Thomas Zimmermann <tzimmerm...@suse.de>
Signed-off-by: Philipp Stanner <pstan...@redhat.com>
---
¡Hola!
I picked up the memory-region-request-task from the DRM-TODO-List [1]
and began with this driver.

Please have a first look. I wasn't entirely sure about -ENOMEM... for
example, as far as my understanding goes, it should not be able to fail
anyways in the second call.

Yes, you can remove these checks, other drivers don't do it:
https://elixir.bootlin.com/linux/latest/source/arch/x86/platform/intel-mid/pwr.c#L372


I don't have the server-hardware, thus, can't test it on a physical
machine.

I've done a quick check on an AST2600, and it works.

Please tell me what you think.

That's a good patch, thanks for your contribution.

I'll wait for Thomas review, and with the checks removed, I can help 
push it to drm-misc-next.


P.

[1] 
https://dri.freedesktop.org/docs/drm/gpu/todo.html#request-memory-regions-in-all-drivers
---
  drivers/gpu/drm/ast/ast_main.c | 15 +++++++++++----
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index dae365ed3969..1004c6628938 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -444,9 +444,13 @@ struct ast_device *ast_device_create(const struct 
drm_driver *drv,
        if (ret)
                return ERR_PTR(ret);
  
-	ast->regs = pcim_iomap(pdev, 1, 0);
+       ret = pcim_iomap_regions(pdev, BIT(1), 0);
+       if (ret)
+               return ERR_PTR(ret);
+
+       ast->regs = pcim_iomap_table(pdev)[1];
        if (!ast->regs)
-               return ERR_PTR(-EIO);
+               return ERR_PTR(-ENOMEM);
You can remove this check.


  
  	/*
         * After AST2500, MMIO is enabled by default, and it should be adopted
@@ -461,9 +465,12 @@ struct ast_device *ast_device_create(const struct 
drm_driver *drv,
  
  	/* "map" IO regs if the above hasn't done so already */
        if (!ast->ioregs) {
-               ast->ioregs = pcim_iomap(pdev, 2, 0);
+               ret = pcim_iomap_regions(pdev, BIT(2), 0);
+               if (ret)
+                       return ERR_PTR(ret);
+               ast->ioregs = pcim_iomap_table(pdev)[2];
                if (!ast->ioregs)
-                       return ERR_PTR(-EIO);
+                       return ERR_PTR(-ENOMEM);
you can remove this check too.
        }
  
  	if (!ast_is_vga_enabled(dev)) {

--

Jocelyn