[alsa-devel] asihpi driver -> kernel

[Takashi Iwai]

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