On 2012-09-24 22:25, H Hartley Sweeten wrote:
On Monday, September 24, 2012 9:47 AM, Ian Abbott wrote:
On 2012-09-24 17:38, H Hartley Sweeten wrote:
On Monday, September 24, 2012 7:20 AM, Ian Abbott wrote:
Tag pointers to remapped I/O memory with `__iomem` and remove the
`volatile` qualifiers. Fix the single place in `jr3_pci_attach()` where
an I/O memory pointer is being dereferenced directly to use the
appropriate I/O memory access function.
Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
Ian,
I don't think this is _completely_ right.
---
drivers/staging/comedi/drivers/jr3_pci.c | 35 ++++++++++++++++----------------
drivers/staging/comedi/drivers/jr3_pci.h | 12 +++++------
2 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/comedi/drivers/jr3_pci.c b/drivers/staging/comedi/drivers/jr3_pci.c
index 360107c..5b193ef 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.c
+++ b/drivers/staging/comedi/drivers/jr3_pci.c
@@ -62,7 +62,7 @@ struct jr3_pci_dev_private {
struct pci_dev *pci_dev;
int pci_enabled;
- volatile struct jr3_t *iobase;
+ struct jr3_t __iomem *iobase;
The ioremap'ed pci base address is a 'void __iomem *'. This is casting
it to a 'struct jr3_t __iomem *'. Which still leaves this driver with the
goofy indirect i/o using the struct instead of normal read[lwb]/write[lwb]
calls.
Changing the iobase variable in the struct like you have done may
get rid of the sparse warning but this drive is still pretty darn confusing.
I think the struct itself needs to be removed an replaced with #define's
for the memory map of the card. Then all the struct i/o access can be
replaced with read*/write* calls as appropriate.
It is actually using readl() and writel() throughout (mostly via its
`get_s16()`, `set_s16()`, get_u16()` and `set_u16()` functions), but
yes, the way the register offsets is determined is a bit goofy!
No. There are places like this:
/* Reset DSP card */
devpriv->iobase->channel[0].reset = 0;
I think I caught that one in my patch. I don't think there are any
others or sparse would have chirped up.
Which should be something like:
#define JR3_RESET_CHAN(x) /* this needs to be determined */
writel(0, devpriv->iobase + JR3_RESET_CHAN(0));
Regards,
Hartley
Sure, it would need a bit of thought to come up with a decent set of
macros or inline functions. One nice feature of mapping the register
layout as a structure is it makes handling the nested offsets quite
intuitive. If the same thing can be modelled nicely with functions (or
function-like macros) and enumerated constants (or macros) then fine.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
