Re: [RFC][PATCH v2]Add pm8001 SAS/SATA HBA driver

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

[-- Attachment #1: Type: Text/Plain, Size: 2661 bytes --]

jack wang wrote:
> Hi, James
> Sorry for the late response. Here is our driver next send with review input
> from Matthew:
> 1 Big endian support is added but not test because lack of equipment.
> 2 IOCTL interface is replaced by sysfs.
> 3 Delete some unused debug info
> 4 Move Vendor id to pci_ids.h
> 5 Kconfig && Makefile modification
> Any comments and reviews will be appreciated
> Best wishes,
> Jack
> 

Just a bunch of random things I found, I didn't look into everything yet:

-LOW_32_BITS/HIGH_32_BITS should be replaced by upper_32_bits/lower_32_bits 
from linux/kernel.h
-there are at least 2 typos where "memory" is written as "memery"

+static void read_inbound_queue_table(struct pm8001_hba_info *pm8001_ha)
+{
+	int inbQ_num = 1;
+	u32 offset;

Can be declared inside the loop.

+	int i;
+	dma_addr_t address = pm8001_ha->inb_addr;
+	for (i = 0; i < inbQ_num; i++) {
+		offset = i * 0x24;
+		pm8001_ha->inb_data[i].pi_pci_bar =
+		get_pci_bar_index(pm8001_mr32(address, (offset + 0x14)));

This need more identation as it's a continued line.

+		pm8001_ha->inb_data[i].pi_offset =
+		pm8001_mr32(address, (offset + 0x18));
+	}
+}

-read_outbound_queue_table(): the same

+ * check_fw_ready - The LLDD check if the FW is ready, if not,wait as long

There is a space mising behind the last ","

+	/* wait until scratch pad 1 and 2 registers in ready state  */
+	do {
+		mdelay(10);
+		value = pm8001_cr32(pm8001_ha, 0, MSGU_SCRATCH_PAD_1)
+			& SCRATCH_PAD1_RDY;
+		value1 = pm8001_cr32(pm8001_ha, 0, MSGU_SCRATCH_PAD_2)
+			& SCRATCH_PAD2_RDY;
+		if ((--max_wait_count) == 0)
+			break;
+	} while ((value != SCRATCH_PAD1_RDY) || (value1 != SCRATCH_PAD2_RDY));
+
+	if (!max_wait_count)
+		return -1;
+	return 0;

Why not directly "return -1" inside the loop?

+	/*This register is a 16-bit timer with a resolution of 1us. This is the
+	timer used for interrupt delay/coalescing in the PCIe Application Layer.
+	Zero is not a valid value.A value of 1 in the register will cause the
                              ^

Space missing

+/**
+ *Function to do BAR shifting function is called to shift BAR base address
+ *
+ *  @pm8001_ha handles for this instance of SAS/SATA hardware
+ *  @shiftValue shifting value
+ *
+ *  return success or fail
+ */

-you should use proper kerneldoc comments
-the description text duplicates itself somehow

+	return 0;
+}
+static int mpi_uninit_check(struct pm8001_hba_info *pm8001_ha)
+{

Newline missing.

There are some more of those I bet. I'll have to leave now, I'm sure someone 
will take care of the rest.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]