Re: [PATCH 2/8] qla4xxx: initialization routines

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: text/plain, Size: 2959 bytes --]

Andrew Vasquez wrote:
>ISP4xxx driver initialization routines.
>
>Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
>---
>
> drivers/scsi/qla4xxx/ql4_init.c | 1753
> +++++++++++++++++++++++++++++++++++++++ drivers/scsi/qla4xxx/ql4_iocb.c | 
> 595 +++++++++++++
> 2 files changed, 2348 insertions(+), 0 deletions(-)
> create mode 100644 drivers/scsi/qla4xxx/ql4_init.c
> create mode 100644 drivers/scsi/qla4xxx/ql4_iocb.c
>
>6755c3b78809194fe31f551b3862639ae513b9a9
>diff --git a/drivers/scsi/qla4xxx/ql4_init.c
> b/drivers/scsi/qla4xxx/ql4_init.c new file mode 100644
>--- /dev/null
>+++ b/drivers/scsi/qla4xxx/ql4_init.c
>@@ -0,0 +1,1753 @@
>+/*
>+ * Copyright (c)  2003-2005 QLogic Corporation
>+ * QLogic Linux iSCSI Driver
>+ *
>+ * This program includes a device driver for Linux 2.6 that may be
>+ * distributed with QLogic hardware specific firmware binary file.
>+ * You may modify and redistribute the device driver code under the
>+ * GNU General Public License as published by the Free Software
>+ * Foundation (version 2 or a later version) and/or under the
>+ * following terms, as applicable:

[huge licence stuff]

Wouldn't it be enough to include that in one of the files (or even better: in 
an extra file in Documentation/scsi/ or somewhere) and then reference to it?


>+/**************************************************************************
>+ * qla4xxx_free_ddb
>+ *	This routine deallocates and unlinks the specified ddb_entry from the
>+ *	adapter's
>+ *
>+ * Input:
>+ * 	ha - Pointer to host adapter structure.
>+ *	ddb_entry - Pointer to device database entry
>+ *
>+ * Returns:
>+ *	None
>+ *
>+ * Context:
>+ *	Kernel context.
>+
> **************************************************************************/

Please take a look at Documentation/kernel-doc-nano-HOWTO.txt. And kernel 
context is pretty normal in a device driver.

>+/*
>+ * qla4xxx_init_rings
>+ *	This routine initializes the internal queues for the specified adapter.
>+ *
>+ * Input:
>+ * 	ha - Pointer to host adapter structure.
>+ *
>+ * Remarks:
>+ *	The QLA4010 requires us to restart the queues at index 0.
>+ *	The QLA4000 doesn't care, so just default to QLA4010's requirement.
>+ * Returns:
>+ *	QLA_SUCCESS - Always return success.
>+ *
>+ * Context:
>+ *	Kernel context.
>+ */

And here you have a different style just 2 functions later

>+int
>+qla4xxx_init_rings(scsi_qla_host_t * ha)
>+{
[...]
>+	/* Initialize active array */
>+	for (i = 0; i < MAX_SRBS; i++)
>+		ha->active_srb_array[i] = 0;
>+	ha->active_srb_count = 0;

memset() ?

>+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
>+
>+	return QLA_SUCCESS;
>+}

I don't see it returning anything different. So this return value is rather 
useless, isn't it? And from my point of view I prefer to use the normal error 
codes as much as possible instead of inventing my own.

Eike


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]