At Tue, 18 Dec 2007 13:16:38 +1300, Eliot Blennerhassett wrote:
Hi Takashi,
thanks for looking over our code. Some of your comments are addressed below.
Takashi Iwai wrote:
One thing I noticed is the function definition/declaration style.
void HPI_GetErrorText( u16 wError, char *pszErrorText ) { ...
It's uncommon. Normally, it's in a single line, and the line starting with the opening brace. See CodingStyle as reference.
Yes. We find that this style leaves room for doxygen comments on each function parameter and keep within 80 columns. In hpifunc.c this is not evident because we are stripping all the comments out for now.
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.
So, one thing I forgot to mention is to avoid /** for non-kerneldoc comments.
(Besides, the hungarian style is hated by many people. Let's avoid it as much as possible.)
Also, in general, the removal of all HpiOs_*() is a good goal. The kernel guys really hate wrappers like that. Let's use native API.
I think all the thin wrappers have been removed. What remain are
- 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...
- 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.
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.
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.
- Replace semaphore with mutex as much as possible.
OK, will look at it. BTW is there an easy way to find out at what kernel version a given API was added/removed/changed?
The alsa-driver tree has a wrapper, so you can use mutex without any checks in the driver.
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
Only hpi_version structure I think? Not used by kernel driver at all, we can make it disappear.
struct hpi_handle has bitfields. I think it's used very commonly in asihpi.
- The way of clean up like hpimod_cleanup() is uncommon. (Above all, passing just a digit is a bad style...)
OK. So replace this with checks like this?:
if (asihpi_class) class_device_destroy(asihpi_class... if (chrdev_registered) unregister_chrdev(... if (asihpi_pci_driver) pci_unregister_driver(...
Yes, it looks better to me.
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
Did you have a list? There are a few function names that don't start with HPI.
Err, sorry, I meant global variables *and functions*.
thanks,
Takashi