Re: [PATCH 1/3] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

2013-08-26 Thread James Bottomley
On Mon, 2013-08-26 at 12:14 +0800, 黃清隆 wrote:
  From: Ching ching2...@areca.com.tw
 
 Support Areca new SATA Raid adapter ARC1214/1224/1264/1284.
 Modify maximum outstanding command number, notify command complete with auto
 request sense
 Signed-off-by: Ching  ching2...@areca.com.tw

Checkpatch really doesn't like this:

total: 1 errors, 173 warnings, 7131 lines checked

I fixed up the error and the few relevant warnings.  I don't really care
about the lines over 80 characters.  However, if you're going to have
long lines, don't split strings (another checkpatch complaint).  I'd
also like to see the spurious braces removed in a later patch, please.
The prinks without a KERN_ also need fixing.

James


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

2013-08-26 Thread Tomas Henzl
On 08/26/2013 06:14 AM, 黃清隆 wrote:
  From: Ching ching2...@areca.com.tw

 Support Areca new SATA Raid adapter ARC1214/1224/1264/1284.
 Modify maximum outstanding command number, notify command complete with auto
 request sense
 Signed-off-by: Ching  ching2...@areca.com.tw

Hi Ching,
+static bool
+arcmsr_hbaD_get_config(struct AdapterControlBlock *acb)
+{
...
+   dma_coherent = dma_alloc_coherent(pdev-dev, acb-uncache_size,
+   dma_coherent_handle, GFP_KERNEL);
+   if (!dma_coherent) {
+   pr_notice(DMA allocation failed...\n);
+   return -ENOMEM;
You've declared the return value as a bool so the function should return false 
or true.
---
In arcmsr_alloc_ccb_pool
-   roundup_ccbsize = roundup(sizeof(struct CommandControlBlock) + 
(max_sg_entrys - 1) * sizeof(struct SG64ENTRY), 32);
-   acb-uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM;
-   dma_coherent = dma_alloc_coherent(pdev-dev, acb-uncache_size, 
dma_coherent_handle, GFP_KERNEL);
-   if(!dma_coherent){
-   printk(KERN_NOTICE arcmsr%d: dma_alloc_coherent got error\n, 
acb-host-host_no);
-   return -ENOMEM;
+   switch (acb-adapter_type) {
+   case ACB_ADAPTER_TYPE_A: {
+   roundup_ccbsize =
+   roundup(sizeof(struct CommandControlBlock) +
+   max_sg_entrys * sizeof(struct SG64ENTRY), 32);
+   acb-uncache_size = roundup_ccbsize *
+   ARCMSR_MAX_FREECCB_NUM;
+   dma_coherent = dma_alloc_coherent(pdev-dev,
+   acb-uncache_size, dma_coherent_handle,
+   GFP_KERNEL);
...
+   case ACB_ADAPTER_TYPE_B: {
+   roundup_ccbsize = roundup(sizeof(struct CommandControlBlock) +
+   (max_sg_entrys - 1) * sizeof(struct SG64ENTRY), 32);
+   acb-uncache_size = roundup_ccbsize *
+   ARCMSR_MAX_FREECCB_NUM;
+   dma_coherent = dma_alloc_coherent(pdev-dev,
+   acb-uncache_size, dma_coherent_handle,
+   GFP_KERNEL);
You've added a switch with  four almost identical
cases, what is the point of splitting the code this way?
-
 struct AdapterControlBlock
+   void *dma_coherent;
+   dma_addr_t dma_coherent_handle;
+   dma_addr_t dma_coherent_handle2;
+   void *dma_coherent2;
why do you need these pairs? Is there an adapter which uses both
for example dma_coherent and dma_coherent2 at the same time?

Please include the patch into the message body next time, it makes the review 
easier.
Thanks, Tomas 
 

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/3] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

2013-08-25 Thread 黃清隆

From: Ching ching2...@areca.com.tw


Support Areca new SATA Raid adapter ARC1214/1224/1264/1284.
Modify maximum outstanding command number, notify command complete with auto 
request sense

Signed-off-by: Ching  ching2...@areca.com.tw

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html