Re: Analog input problem with MultiQ PCI daq card

On 01/08/2006 04:37, Farhan Rizwi wrote:
> On Mon, 31 Jul 2006 08:37 pm, Ian Abbott wrote:
>> On 28/07/06 23:42, Farhan Rizwi wrote:
>>> On Fri, 28 Jul 2006 08:16 pm, you wrote:
>>>> Which driver are you using?  I cannot see any driver in the standard
>>>> Comedi package that includes support for multiqpci.  The closest match
>>>> is the multiq3 driver, but that one is for an ISA card, not a PCI card.
>>> A few years ago someone in my lab ported it from Windows.  Its attached.
>> Do they still work there and can they fix it, or has it been dumped on
>> you to get working?
> 
> No they don't work here anymore.  

That's a shame!

>> I suggest you rewrite the parts of the driver that use the above types
>> COUNTER_SETUP, CRB_LAYOUT, and CRA_LAYOUT to avoid the use of
>> bit-fields.  Using bit-fields to describe hardware register layouts is
>> not a good idea because they are inherently non-portable for externally
>> defined layouts.  In fact, the order of the bits in the above types
>> appears to be the reverse of what it should be for gcc on i386.  Perhaps
>> it was originally ported to an architecture that lays the bit-fields out
>> MSB-first.  Besides, the driver code currently uses a confusing mixture
>> of bit-fields and bit-wise logical and shift operations, so changing the
>> code to avoid bit-fields would make it easier to understand.
> 
> I understand what you're saying but if this is the same code (there is some 
> confusion on this) that is being executed on win32 then the bits ought to 
> line up in linux too, right? Unless of course, Borland compiles differently 
> to gcc.

For x86 architecture, gcc, bcc and cl all fill bit-fields LSB-first, as 
can be proved by compiling and running a simple test program.  But your 
driver code seems to be expecting them to be filled MSB-first.

For example:

#define CRABIT_INDXSRC_B		14		//   B index source.
#define CRABIT_INDXPOL_A		11		//   A index polarity.
#define CRABIT_LOADTRIG_A		 9		//   A preload trigger.
#define CRABIT_INTSRC_A			 5		//   A interrupt source.

#define CRA_INDXSRC_B		(uint16_t)( 3 << CRABIT_INDXSRC_B	)
#define CRA_CLKSRC_B		(uint16_t)( 3 << CRBBIT_INTSRC_B	)
#define CRA_INDXPOL_A		(uint16_t)( 1 << CRABIT_INDXPOL_A	)
#define CRA_LOADTRIG_A		(uint16_t)( 3 << CRABIT_LOADTRIG_A	)
#define CRA_INTSRC_A		(uint16_t)( 3 << CRABIT_INTSRC_A	)

// Layout of CRA register.
typedef union {
	uint16_t wval;
	struct {
		//#elif BITORDER == MSB_FIRST	//---------------------------
			unsigned BIndexSrc	: 2;	// B index source.
			unsigned BClkSrc	: 2;	// B clock source.
			unsigned AIndexPol	: 1;	// A index polarity.
			unsigned ALoadSrc	: 2;	// A preload trigger source.
			unsigned AClkMult	: 2;	// A clock multiplier.
			unsigned AIntSrc	: 2;	// A interrupt source.
			unsigned AClkPol	: 1;	// A clock polarity.
			unsigned AIndexSrc	: 2;	// A index source.
			unsigned AClkSrc	: 2;	// A clock source (LSB).
	} bits;
} CRA_LAYOUT;

When CRA_LAYOUT.bits is layed out LSB-first, AIntSrc starts at bit 9, 
but CRABIT_INTSRC_A says it starts at bit 5.  Similarly, ALoadSrc starts 
at bit 5, but CRABIT_LOADTRIG_A says bit 9; AIndexPol starts at bit 4, 
but CRABIT_INDXPOL_A says bit 11; BIndexSrc starts at bit 0, but 
CRABIT_INDXSRC_B says bit 14.

> In any case, the counters seem to be working okay, its the Analog input I'm 
> having trouble with.  I thought the DMABuffer might have an issue with struct 
> alignment as m_pBuffer is declared as uint8 and then cast to uint32 
> everywhere else but I'm not sure.  ReadADC simply accesses a bit of the DMA 
> buffer so unless that pointer is initialized incorrectly, I don't know where 
> else to look.

The alignment should be okay as pci_alloc_consistent returns a page-aligned 
buffer (at least for i386).  Also, the buffer is cache-coherent between the 
CPU and DMA master, so there is no need to flush any caches before or after 
accessing the buffer.

There are probably some endian issues with the  casting to uint32_t * and 
then modifying the buffer through that pointer, i.e. the buffer contents will 
be set differently for big-endian and little-endian architectures.  However, 
I assume the code is correct for little-endian architectures such as i386, 
so that doesn't explain why it's not working.  (Making it work on big-endian 
architectures too is sometimes as simple as turning on the right bits in the 
PCI interface chip's registers to perform the endian conversions 
automatically.)

I don't know enough about the hardware to comment on why it isn't working, 
and besides, it looks complicated!

The code in MQPCI_attach and MQPCI_detach that deals with PCI devices and 
resources could do with a bit of modernization and the addition of some 
error-checking code before it is contributed to the Comedi project (if at 
all), but that can wait until the driver is basically working (at least 
working for the more common, little-endian architectures such as i386). 
The attached patch should take care of that, but won't work for Comedi 
versions before comedi-0.7.70 on 2.4 kernels due to lack of support for 
the pci_get_subsys and pci_dev_put functions.  The patch also adds 
support for specifying the PCI bus and slot as comedi_config options so 
the driver can support more than one device.

-- 
-=( Ian Abbott _at_ MEV Ltd.    E-mail: <abbotti_at_mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-