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