Grant, For your comment #1, Print is just extra debug print. I am agreeing with your concern. I will revert back that particular print, since it is duplicate information. For your comment #2, Black space will be removed in next patch set. For your comment #3, it is just code readability purpose. I would like to keep that piece of code, since the new way is more readable. I can revert back that particular piece of code, if it is strongly recommended. Thanks, Kashyap -----Original Message----- From: Grant Grundler [mailto:grundler@xxxxxxxxxx] Sent: Thursday, April 16, 2009 12:22 PM To: Desai, Kashyap Cc: linux-scsi@xxxxxxxxxxxxxxx; James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; Moore, Eric; Prakash, Sathya Subject: Re: [PATCH 1/1mpt fusion:C] Code cleanup patch On Wed, Apr 15, 2009 at 4:10 AM, Kashyap, Desai <kashyap.desai@xxxxxxx> wrote: > Main goal to submit this patch is code cleaup. > 1. Better driver debug prints and code indentation. A few silly nits about the new printk's, etc. ... > @@ -1596,6 +1600,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > u8 revision; > int r = -ENODEV; > struct pci_dev *pdev; > + struct sysinfo s; > > pdev = ioc->pcidev; > ioc->bars = pci_select_bars(pdev, IORESOURCE_MEM); > @@ -1647,6 +1652,11 @@ mpt_mapresources(MPT_ADAPTER *ioc) > } > } > > + si_meminfo(&s); > + printk(MYIOC_s_INFO_FMT "%s BIT PCI BUS DMA ADDRESSING SUPPORTED, " > + "total memory = %ld kB\n", > + ioc->name, ioc->dma_mask == DMA_64BIT_MASK ? "64" : "32", > + convert_to_kb(s.totalram)); 1) Is printing one line for dma_mask really necessary? Maybe consolidate this onto another line of output. Boot output is already too verbose. 2) Isn't the "total Ram" printed elsewhere at boot time? Will this get printed once for each MPT Adapter in the system? Lastly, it's not the total RAM that matters, it depends on *where* in the physical address space that RAM is located. I'm sure there are better ways to get that than by a device driver printing it. We have lots of systems with 4G of RAM and some major chunk of that is above 4GB addresss. I forget if it's 1GB or 2GB on x86 systems. That is to give more MMIO address space to PCI devices that want to live below 4GB phys address and possibly use by IOMMUs (I don't know that for x86 - it was the case for HP IA64 systems). > @@ -1768,7 +1779,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > ioc->reply_sz = MPT_REPLY_FRAME_SIZE; > > ioc->pcidev = pdev; > - spin_lock_init(&ioc->initializing_hba_lock); > + > > spin_lock_init(&ioc->taskmgmt_lock); > mutex_init(&ioc->internal_cmds.mutex); Extra blank line needed? Could probably be dropped. > @@ -1789,6 +1800,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > ioc->mfcnt = 0; > #endif > > + ioc->sh = NULL; > ioc->cached_fw = NULL; > > /* Initilize SCSI Config Data structure > @@ -1805,9 +1817,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > > /* Initialize workqueue */ > INIT_DELAYED_WORK(&ioc->fault_reset_work, mpt_fault_reset_work); > - spin_lock_init(&ioc->fault_reset_work_lock); > > - snprintf(ioc->reset_work_q_name, sizeof(ioc->reset_work_q_name), > + snprintf(ioc->reset_work_q_name, MPT_KOBJ_NAME_LEN, > "mpt_poll_%d", ioc->id); > ioc->reset_work_q = > create_singlethread_workqueue(ioc->reset_work_q_name); > @@ -1882,11 +1893,14 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > case MPI_MANUFACTPAGE_DEVID_SAS1064: > case MPI_MANUFACTPAGE_DEVID_SAS1068: > ioc->errata_flag_1064 = 1; > + ioc->bus_type = SAS; > + break; Why not Just let it fall through to the next case? Did someone have problems reading that code? > > case MPI_MANUFACTPAGE_DEVID_SAS1064E: > case MPI_MANUFACTPAGE_DEVID_SAS1068E: > case MPI_MANUFACTPAGE_DEVID_SAS1078: > ioc->bus_type = SAS; > + break; > } > > thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
