Re: [PATCH 12/12] ahci: Add platform driver

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

Anton Vorontsov wrote:

> This can be used for AHCI-compatible interfaces implemented inside
> a System-On-Chip solutions, or AHCI devices connected via localbus.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
[...]
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> new file mode 100644
> index 0000000..f3f96d6
> --- /dev/null
> +++ b/drivers/ata/ahci_platform.c
> @@ -0,0 +1,196 @@
> +/*
> + * AHCI SATA platform driver
> + *
> + * Copyright 2004-2005  Red Hat, Inc.
> + *   Jeff Garzik <jgarzik@pobox.com>
> + * Copyright 2010  MontaVista Software, LLC.
> + *   Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/libata.h>
> +#include <linux/platform_device.h>
> +#include "ahci.h"
> +#include "ahci_platform.h"
> +
> +static int __devinit ahci_probe(struct platform_device *pdev)
>   

  Does it make sense to make this driver hotplug capable?

> +{
> +	struct device *dev = &pdev->dev;
> +	struct ahci_platform_data *pdata = pdev->dev.platform_data;
> +	struct ata_port_info pi = {
> +		.flags		= AHCI_FLAG_COMMON,
> +		.pio_mask	= ATA_PIO4,
> +		.udma_mask	= ATA_UDMA6,
> +		.port_ops	= &ahci_ops,
> +	};
> +	const struct ata_port_info *ppi[] = { &pi, NULL };
> +	struct ahci_host_priv *hpriv;
> +	struct ata_host *host;
> +	struct resource *res;
> +	int n_ports;
> +	int i;
> +	int rc;
> +
> +	WARN_ON(ATA_MAX_QUEUE > AHCI_MAX_CMDS);
>   

   Hm...

> +	ahci_set_em_messages(hpriv, &pi);
> +
> +
>   

   Extra newline?

> +
> +
>   

  Another one?

> +		/* disabled/not-implemented port */
> +		if (!(hpriv->port_map & (1 << i)))
> +			ap->ops = &ata_dummy_port_ops;
> +	}
> +
> +	rc = ahci_reset_controller(host);
> +	if (rc)
> +		goto err0;
> +
> +	ahci_init_controller(host);
> +	ahci_print_info(host, "platform");
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>   

   Could also use platform_get_irq()...

> diff --git a/drivers/ata/ahci_platform.h b/drivers/ata/ahci_platform.h
> new file mode 100644
> index 0000000..f7dd576
> --- /dev/null
> +++ b/drivers/ata/ahci_platform.h
>   

   I doubt that it's good place for declaring the platform data which 
will be used by the platform code. This file should be somewhere in 
include/linux/, don't you think?

> @@ -0,0 +1,29 @@
> +/*
> + * AHCI SATA platform driver
> + *
> + * Copyright 2004-2005  Red Hat, Inc.
> + *   Jeff Garzik <jgarzik@pobox.com>
> + * Copyright 2010  MontaVista Software, LLC.
> + *   Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + */
> +
> +#ifndef _AHCI_PLATFORM_H
> +#define _AHCI_PLATFORM_H
> +
> +struct device;
> +struct ata_port_info;
> +
> +struct ahci_platform_data {
> +	int (*init)(struct device *dev);
> +	void (*exit)(struct device *dev);
> +	const struct ata_port_info *ata_port_info;
> +	unsigned int force_port_map;
> +	unsigned int mask_port_map;
> +};
> +
> +#endif /* _AHCI_PLATFORM_H */
>   

MBR, Sergei