Re: [PATCH] ahci_platform: Provide for vendor specific init

[Anton Vorontsov <cbouatmailru@gmail.com>]

On Fri, Jun 25, 2010 at 01:57:38PM +0900, Jassi Brar wrote:
> From: Jassi Brar <jassi.brar@samsung.com>
> 
> Some AHCI implementations may use Vendor Specific HBA[A0h, FFh]
> and/or Port[70h, 7Fh] registers to 'prepare' for initialization.
> For that, the platform needs memory mapped address of AHCI registers.
> 
> This patch adds the 'mmio' argument and reorders the call to
> platform init function.
> 
> Signed-off-by: Jassi Brar <jassi.brar@samsung.com>

Thanks for the patch. Looks good, but see some comments below.
(Plus some comments on Jeff's reply)

> ---
>  drivers/ata/ahci_platform.c   |   23 +++++++++++++----------
>  include/linux/ahci_platform.h |    2 +-
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 5e11b16..4403941 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
[...]
> @@ -74,8 +68,17 @@ static int __init ahci_probe(struct platform_device *pdev)
>  	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
>  	if (!hpriv->mmio) {
>  		dev_err(dev, "can't map %pR\n", mem);
> -		rc = -ENOMEM;
> -		goto err0;
> +		devm_kfree(dev, hpriv);

You don't need to explicitly free devm_* resources.

> +		return -ENOMEM;
> +	}
> +
> +	if (pdata && pdata->init) {
> +		rc = pdata->init(dev, hpriv->mmio);
> +		if (rc) {
> +			devm_iounmap(dev, hpriv->mmio);
> +			devm_kfree(dev, hpriv);

Ditto, no need for iounmap and kfree.

> +			return rc;
> +		}
>  	}
>  
>  	ahci_save_initial_config(dev, hpriv,
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index f7dd576..f7ffc4c 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -19,7 +19,7 @@ struct device;
>  struct ata_port_info;
>  
>  struct ahci_platform_data {
> -	int (*init)(struct device *dev);
> +	int (*init)(struct device *dev, void __iomem *addr);

Please include linux/compiler.h for __iomem.

>  	void (*exit)(struct device *dev);
>  	const struct ata_port_info *ata_port_info;
>  	unsigned int force_port_map;

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2