[alsa-devel] asihpi driver -> kernel
tiwai at suse.de
Wed Dec 19 14:33:50 CET 2007
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
It's fine to do this in user context (unless HPI_somthing() takes too
> >> 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.
More information about the Alsa-devel