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