[alsa-devel] asihpi driver -> kernel
tiwai at suse.de
Fri Dec 14 17:09:42 CET 2007
At Fri, 07 Dec 2007 12:33:23 +1300,
Eliot Blennerhassett wrote:
> Eliot Blennerhassett wrote:
> > Working on it, will submit new patches in the next week or so.
> Hi Takashi,
> I have been beating our code with a big stick for the last 4 weeks.
> I'd appreciate any comment you have on the progress.
Thanks for your work. Now I committed the codes below to HG tree,
so further work could be provided as patches.
> You can see the result here:
> The matching firmware is here:
> Note that dsp6413.bin is replaced by dsp6400.bin, dsp8713 by dsp8700.
> You can remove all the dsp*.txt from the repo they are not needed. Also
> remove dsp2400, its not relevant.
> The code is copmplete files rather than diffs, there are so many diffs I
> don't think it is useful. Apart from cosmetic changes, there are some
> added and removed files compared to curren ALSA Hg repo:
> * Merged all files relating to HPI4000 into hpi4000.[ch] (remove
> hpi56301.[ch], boot4ka.h, dpi56301.h
> * Removed hpicheck.h
> * add hpimsginit.[ch] (no new functions)
> Cosmetic changes
> * Code is mostly 'checkpatch clean',
> though still some real work to do
> around our use of volatile for structures that are updated by DMA by our
Yeah, volatile is a thing that is often ambiguous and fishy.
> * Only 2 'sparse' warnings
> * Got rid of nearly all typedefs, and most thin wrappers around kernel
One thing I noticed is the function definition/declaration style.
It's uncommon. Normally, it's in a single line, and the line starting
with the opening brace. See CodingStyle as reference.
(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
Other things I noticed:
- The ioctl of hpi chrdev is dangerous: it may cause Oops when
accessing to a non-existing adapter. 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.
- Replace semaphore with mutex as much as possible.
- Don't use bitfields for data communication. It's higly unportable.
Use explit bit shift and mask instead.
- The way of clean up like hpimod_cleanup() is uncommon.
(Above all, passing just a digit is a bad style...)
- Remove HPI_UNUSED(). We don't check unused parameters.
- Global variables should have some unique prefix. The kernel code is
monolithc C, so they are really global and has no name space.
- HPI_GetErrorText() is dangerous. It may overflow easily.
Don't use strcat() unless you are really sure. Use variants with
the size limit.
More information about the Alsa-devel