Re: [PATCH 04/21] ata: libahci_platform: Convert to using handy devm-ioremap methods

[Damien Le Moal]

On 3/24/22 09:16, Serge Semin wrote:
> Currently the IOMEM AHCI registers space is mapped by means of the
> two functions invocation: platform_get_resource() is used to get the very
> first memory resource and devm_ioremap_resource() is called to remap that
> resource. Device-managed kernel API provides a handy wrapper to perform
> the same in single function call: devm_platform_ioremap_resource().

> 
> While at it seeing many AHCI platform drivers rely on having the AHCI CSR
> space marked with "ahci" name let's first try to find and remap the CSR
> IO-mem with that name and only if it fails fallback to getting the very
> first registers space platform resource.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/ata/libahci_platform.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 1bd2f1686239..8eabbb5f208c 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -404,11 +404,13 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  
>  	devres_add(dev, hpriv);
>  
> -	hpriv->mmio = devm_ioremap_resource(dev,
> -			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +	hpriv->mmio = devm_platform_ioremap_resource_byname(pdev, "ahci");

See __devm_ioremap_resource(): if there is no resource named "ahci" found,
then this will print an error message ("invalid resource\n"). That may
confuse users as this error message was not present before. So you may
want to change this code to something like this:

/*
 * If the DT provided an "ahci" named resource, use it. Otherwise,
 * fallback to using the default first resource for the device node.
 */
if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci"))
	hpriv->mmio = devm_platform_ioremap_resource_byname(pdev, "ahci");
else
	hpriv->mmio = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(hpriv->mmio)) {
	rc = PTR_ERR(hpriv->mmio);
	goto err_out;
}

>  	if (IS_ERR(hpriv->mmio)) {
> -		rc = PTR_ERR(hpriv->mmio);
> -		goto err_out;
> +		hpriv->mmio = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(hpriv->mmio)) {
> +			rc = PTR_ERR(hpriv->mmio);
> +			goto err_out;
> +		}
>  	}
>  
>  	for (i = 0; i < AHCI_MAX_CLKS; i++) {


-- 
Damien Le Moal
Western Digital Research