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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
