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;
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
