[patch] staging: comedi: gsc_hpdi plx9080: Resolve sparse "different base types" warnings.

[patch] staging: comedi: gsc_hpdi plx9080: Resolve sparse "different base types" warnings.

[Shaun Laing]

Hi all,

Do you see anything wrong with submitting this patch?  My primary concern is
mixing the functional changes with the aesthetic changes.

Thanks.

* Resolves sparse warnings of the form 
	"warning: incorrect type in assignment (different base types)".
* Adds byte swapping to DEBUG_PRINT statements.
* Removes some spaces to elimite "checkpatch" warnings.

Signed-off-by: Shaun Laing <shaun@xresource.ca>
---
diff --git a/drivers/staging/comedi/drivers/gsc_hpdi.c b/drivers/staging/comedi/drivers/gsc_hpdi.c
index cdcc8f4..ca96af1 100644
--- a/drivers/staging/comedi/drivers/gsc_hpdi.c
+++ b/drivers/staging/comedi/drivers/gsc_hpdi.c
@@ -437,18 +437,18 @@ static int setup_dma_descriptors(struct comedi_device *dev,
 
 		DEBUG_PRINT(" desc %i\n", i);
 		DEBUG_PRINT(" start addr virt 0x%p, phys 0x%lx\n",
-			    devpriv->desc_dio_buffer[i],
-			    (unsigned long)devpriv->dma_desc[i].
-			    pci_start_addr);
+			devpriv->desc_dio_buffer[i],
+			(unsigned long)le32_to_cpu(devpriv->dma_desc[i].
+			pci_start_addr));
 		DEBUG_PRINT(" next 0x%lx\n",
-			    (unsigned long)devpriv->dma_desc[i].next);
+			(unsigned long)le32_to_cpu(devpriv->dma_desc[i].next));
 	}
 	devpriv->num_dma_descriptors = i;
 	/*  fix last descriptor to point back to first */
 	devpriv->dma_desc[i - 1].next  	    cpu_to_le32(devpriv->dma_desc_phys_addr | next_bits);
 	DEBUG_PRINT(" desc %i next fixup 0x%lx\n", i - 1,
-		    (unsigned long)devpriv->dma_desc[i - 1].next);
+		    (unsigned long)le32_to_cpu(devpriv->dma_desc[i - 1].next));
 
 	devpriv->block_size = transfer_size;
 
@@ -795,8 +795,8 @@ static void drain_dma_buffers(struct comedi_device *dev, unsigned int channel)
 		devpriv->dma_desc_index %= devpriv->num_dma_descriptors;
 
 		DEBUG_PRINT("next desc addr 0x%lx\n", (unsigned long)
-			    devpriv->dma_desc[devpriv->dma_desc_index].
-			    next);
+			le32_to_cpu(devpriv->dma_desc[devpriv->dma_desc_index].
+			next));
 		DEBUG_PRINT("pci addr reg 0x%x\n", next_transfer_addr);
 	}
 	/*  XXX check for buffer overrun somehow */
diff --git a/drivers/staging/comedi/drivers/plx9080.h b/drivers/staging/comedi/drivers/plx9080.h
index 0d254a1..dda878b 100644
--- a/drivers/staging/comedi/drivers/plx9080.h
+++ b/drivers/staging/comedi/drivers/plx9080.h
@@ -29,13 +29,13 @@
 
 /*  descriptor block used for chained dma transfers */
 struct plx_dma_desc {
-	volatile uint32_t pci_start_addr;
-	volatile uint32_t local_start_addr;
+	volatile __le32 pci_start_addr;
+	volatile __le32 local_start_addr;
 	/* transfer_size is in bytes, only first 23 bits of register are used */
-	volatile uint32_t transfer_size;
+	volatile __le32 transfer_size;
 	/* address of next descriptor (quad word aligned), plus some
 	 * additional bits (see PLX_DMA0_DESCRIPTOR_REG) */
-	volatile uint32_t next;
+	volatile __le32 next;
 };
 
 /**********************************************************************

Re: [patch] staging: comedi: gsc_hpdi plx9080: Resolve sparse "different base types" warnings.

[Dan Carpenter]

On Thu, Aug 01, 2013 at 11:44:15AM -0600, Shaun Laing wrote:
> Hi all,
> 
> Do you see anything wrong with submitting this patch?  My primary concern is
> mixing the functional changes with the aesthetic changes.
> 
> Thanks.
> 
> * Resolves sparse warnings of the form 
> 	"warning: incorrect type in assignment (different base types)".

It feels like the ->dma_desc buffer is supposed to go to be used
by the hardware but I don't see where that happens.  This driver
looks like complete garbage.  We should re-write it completely.
It's better to leave the static checker warnings there to mark that
the driver needs to be re-written instead of silencing the warnings
but not fixing the code.  Fixing the code proably means re-writing
it to be CPU endian.

> * Adds byte swapping to DEBUG_PRINT statements.

These debug statments are just noise.  Delete them instead of
modifying them.

> * Removes some spaces to elimite "checkpatch" warnings.

I don't see any checkpatch warnings.  I assume you are talking about
the indent changes here.

>  		DEBUG_PRINT(" start addr virt 0x%p, phys 0x%lx\n",
> -			    devpriv->desc_dio_buffer[i],
> -			    (unsigned long)devpriv->dma_desc[i].
> -			    pci_start_addr);
> +			devpriv->desc_dio_buffer[i],
> +			(unsigned long)le32_to_cpu(devpriv->dma_desc[i].
> +			pci_start_addr));

The indenting was correct in the original and the patch is wrong.

regards,
dan carpenter

Re: [patch] staging: comedi: gsc_hpdi plx9080: Resolve sparse "different base types" warnings.

[Shaun Laing]

Thank you, Dan.

> It feels like the ->dma_desc buffer is supposed to go to be used
> by the hardware but I don't see where that happens.  This driver
> looks like complete garbage.  We should re-write it completely.
> It's better to leave the static checker warnings there to mark that
> the driver needs to be re-written instead of silencing the warnings
> but not fixing the code.  Fixing the code proably means re-writing
> it to be CPU endian.

Ok -- I'll ditch this patch.

>> * Removes some spaces to elimite "checkpatch" warnings.
>
> I don't see any checkpatch warnings.  I assume you are talking about
> the indent changes here.
>
> The indenting was correct in the original and the patch is wrong.

So that I know for the future:  if I remove the indentation changes
'checkpatch' returns these warnings.  Should I ignore these in the
future?

WARNING: line over 80 characters
#15: FILE: drivers/staging/comedi/drivers/gsc_hpdi.c:444:
+                           (unsigned
long)le32_to_cpu(devpriv->dma_desc[i].next));

WARNING: line over 80 characters
#33: FILE: drivers/staging/comedi/drivers/gsc_hpdi.c:798:
+
le32_to_cpu(devpriv->dma_desc[devpriv->dma_desc_index].

Re: [patch] staging: comedi: gsc_hpdi plx9080: Resolve sparse "different base types" warnings.

[Dan Carpenter]

On Fri, Aug 02, 2013 at 07:11:17AM -0600, Shaun Laing wrote:
> Thank you, Dan.
> 
> > It feels like the ->dma_desc buffer is supposed to go to be used
> > by the hardware but I don't see where that happens.  This driver
> > looks like complete garbage.  We should re-write it completely.
> > It's better to leave the static checker warnings there to mark that
> > the driver needs to be re-written instead of silencing the warnings
> > but not fixing the code.  Fixing the code proably means re-writing
> > it to be CPU endian.
> 
> Ok -- I'll ditch this patch.
> 
> >> * Removes some spaces to elimite "checkpatch" warnings.
> >
> > I don't see any checkpatch warnings.  I assume you are talking about
> > the indent changes here.
> >
> > The indenting was correct in the original and the patch is wrong.
> 
> So that I know for the future:  if I remove the indentation changes
> 'checkpatch' returns these warnings.  Should I ignore these in the
> future?
> 

Ah, I get it.  Adding a call to le32_to_cpu() made the line too long
so you pulled it in.

The "right way" to fix this in this case is to delete the debug
message.  Another right way would be to add a temporary variable.
Also the casting is bogus and removing it saves space.

	desc = &devpriv->dma_desc[i];
	DEBUG("blah blah %x", le32_to_cpu(desc->next));

regards,
dan carpenter