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:
signature.asc
Description: This is a digitally signed message part.
