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.
(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
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 */
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
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Other things I noticed:
- The ioctl of hpi chrdev is dangerous: it may cause Oops when accessing to a non-existing adapter.
A bug to fix
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?
- 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?
- 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.
- 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(...
- Remove HPI_UNUSED(). We don't check unused parameters.
OK.
- 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.
- HPI_GetErrorText() is dangerous. It may overflow easily. Don't use strcat() unless you are really sure. Use variants with the size limit.
Valid point.
We can get rid of it from kernel driver entirely by using just error number instead.
regards
Eliot Blennerhassett AudioScience Inc.