Re: [RFC/PATCH 5/16] Ops based MSI implementation

[Greg KH <greg@kroah.com>]

On Thu, Jan 25, 2007 at 07:34:09PM +1100, Michael Ellerman wrote:
> --- /dev/null
> +++ msi/include/linux/msi-ops.h
> @@ -0,0 +1,168 @@
> +/*
> + * Copyright 2006-2007, Michael Ellerman, IBM Corporation.
> + *
> + * 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 of the License, or (at your option) any later version.

Are you sure of the "any later version" part?

> + */
> +
> +#ifndef LINUX_MSI_OPS_H
> +#define LINUX_MSI_OPS_H
> +
> +#ifdef __KERNEL__
> +#ifndef __ASSEMBLY__

These two ifdefs aren't needed.

> +
> +#include <linux/pci.h>
> +#include <linux/msi.h>

Why not just put this structure in the msi.h file?

> +
> +/*
> + * MSI and MSI-X although different in some details, are also similar in
> + * many respects, and ultimately achieve the same end. Given that, this code
> + * tries as far as possible to implement both MSI and MSI-X with a minimum
> + * of code duplication. We will use "MSI" to refer to both MSI and MSI-X,
> + * except where it is important to differentiate between the two.
> + *
> + * Enabling MSI for a device can be broken down into:
> + *  1) Checking the device can support the type/number of MSIs requested.
> + *  2) Allocating irqs for the MSIs and setting up the irq_descs.
> + *  3) Writing the appropriate configuration to the device and enabling MSIs.
> + *
> + * To implement that we have the following callbacks:
> + *  1) check(pdev, num, msix_entries, type)
> + *  2) alloc(pdev, num, msix_entries, type)
> + *  3) enable(pdev, num, msix_entries, type)
> + *	a) setup_msi_msg(pdev, msix_entry, msi_msg, type)
> + *
> + * We give platforms full control over the enable step. However many
> + * platforms will simply want to program the device using standard PCI
> + * accessors. These platforms can use a generic enable callback and define
> + * a setup_msi_msg() callback which simply fills in the "magic" address and
> + * data values. Other platforms may leave setup_msi_msg() empty.
> + *
> + * Disabling MSI requires:
> + *  1) Disabling MSI on the device.
> + *  2) Freeing the irqs and any associated accounting information.
> + *
> + * Which maps directly to the two callbacks:
> + *  1) disable(pdev, num, msix_entries, type)
> + *  2) free(pdev, num, msix_entries, type)
> + */


Please use the proper kernel-doc format so the tools pick up this
documentation automatically.

> +#define msi_debug(fmt, args...)	\
> +	pr_debug("MSI:%s:%d: " fmt, __FUNCTION__, __LINE__, ## args)

Please use dev_dbg(), it makes it easier to track which device is being
referenced.

thanks,

greg k-h