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. > 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! >> 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. 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. >> 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. > >> 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) > 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. >>> - 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). IMO it improves readability, and let the compiler figure out the bit shift and mask stuff. regards Eliot _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
