[alsa-devel] asihpi driver -> kernel

[Takashi Iwai]

At Wed, 19 Dec 2007 09:52:08 +1300,
Eliot Blennerhassett wrote:
> 
> Takashi Iwai wrote:
> > At Tue, 18 Dec 2007 13:16:38 +1300,
> > Eliot Blennerhassett wrote:
> 
> > Well, the doxygen comment is another issue.  Usually, the kerneldoc
> > allows you to write (semi-)javadoc style comments for each function.
> > It begins with "/**" and has standard "@parameter description" lines.
> 
> This breaks "define things in only one place" rule. I.e. my style
> doesn't require the parameter name etc to be repeated in the comment.

Then you shouldn't use kerneldoc (javadoc) thingy...

> > So, one thing I forgot to mention is to avoid /** for non-kerneldoc
> > comments.
> 
> What! just when I think I have everything covered, another picky
> requirement pops up. When will this end!

The /** comment isn't strict, but it's just confusing.  People do
think that it would be a javadoc comment.


> >> I think all the thin wrappers have been removed. What remain are
> >>
> >> 1) Spinlock wrappers, the comments in the code tell why
> >> /* The reason for all this evilness is that ALSA calls some of a drivers
> >>  * operators in atomic context, and some not.  But all our functions channel
> >>  * through the HPI_Message conduit, so we can't handle the different context
> >>  * per function
> >>  */
> > 
> > Then this shouldn't be expressed as a spinlock wrapper.  You cannot
> > use a single lock for that.  And the way you hack doesn't look correct
> > for schedulable contexts...
> 
> AFAIK it works.

spin_lock_bh() is to protect data for user-context and softirqs...

>  We can do away with it if you can tell me how to stop
> alsa midlevel calling HPI functions in both schedulable and
> non-schedulable contexts.

Hm?  If you'd like to unify the call, then choose always the
spin-locked version,

	spin_lock_irqsave(&lock, flags);
	HPI_something();
	spin_unlock_irqrestore(&lock, flags);

It's fine to do this in user context (unless HPI_somthing() takes too
long time).


> >> 2) LockedMem wrappers.  These provide tracking for the DMA areas
> >> Need to keep track of all the info about the mem area so it can be freed
> >> again at the end
> > 
> > Hm, but still I don't see the reason to use kmem_cache there.
> 
> Its a convenient way to allocate however many small chunks are needed.
> Otherwise we need to statically allocate or malloc enough space for
> every possible stream on every possible card. I.e. 32 streams * 16 cards.
> 
> Or can do a major redesign of this part and absorb the DMA mem tracking
> into per-adapter data structures. Not gonna happen soon here.

OK, then we can leave as is.


> >> E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size,
> >> 			pMemArea->cpu_addr, pMemArea->dma_addr);
> > 
> > Use dma_alloc_coherent() / dma_free_coherent() instead.
> > pci_*_consistent() are obsolete.
> > 
> >>> I feel the chrdev should be
> >>>   rather bound to the existing ALSA device, e.g. using hwdep instead.
> >>>   Then the whole story about registration would become more straight.
> >> ...yes I'm interested.
> >> Currently HPI userspace lib just works whether ALSA or plain HPI driver
> >> is loaded. How would this work with your scenario?
> > 
> > The independent HPI driver doesn't exist with your current version
> > (all merged to snd-asihpi).  So, this question sounds irrelevant as
> > now.
> 
> It exists for those who build it themselves or get it from 3rd party
> repos.  We have a number of applications that use HPI, we still want to
> be able to use them to test the cards etc, even if ALSA driver is loaded.
> 
> For our existing customers, HPI has been around much longer than ALSA,
> so we need a transitional period where both interfaces are available
> simultaneously. I'd say about 5 years from when the driver is accepted
> into the kernel should be sufficient.
> 
> (For example Rivendell http://www.rivendellaudio.org/ radio automation
> uses HPI to play/record mpeg audio using our on-card processing)

The hwdep device can be set up to emulate (accept) HPI ioctls.  It's a
matter of device name.

> > If we *really* need to provide an independent HPI access, then hpi
> > should have its own device (most easily via platform device).  But, I
> > feel it's just give compliexity in the end.
> 
> Another possibility is to make HPI access via optional loadable module.
> ALSA driver would only need to export HPI_Message() for this to be
> workable.  Already need to export HPI_Message to support our V4L driver.

This is somewhat different...

> >>> - Don't use bitfields for data communication.  It's higly unportable.
> >>>   Use explit bit shift and mask instead.
> >
> > struct hpi_handle has bitfields.  I think it's used very commonly in
> > asihpi.
> 
> The bitfields are all created and used by the same code module,  they
> are never referenced outside hpifunc.c (so its not "data communication"
> in my book).

This struct is used for data communication between the hardware and
the driver, no?

> IMO it improves readability, and let the compiler figure out the bit
> shift and mask stuff.

How bitfields are assigned is undefined in C.  It works just
coincidentally right now.
If you need to access a certain bit in bytes, you must use the
explicity bit-shift & mask operation, not via bitfields.  The only
purpose of bitfields is to save some bytes for small data.


Takashi