[alsa-devel] asihpi driver -> kernel

[Takashi Iwai]

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:
> http://audioscience.com/internet/download/beta/alsahg3459_asihpi30905.tar.gz
> 
> The matching firmware is here:
> http://audioscience.com/internet/download/beta/dspbins30905.tar.bz2
> 
> 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',

That's great.

> though still some real work to do
> around our use of volatile for structures that are updated by DMA by our
> adapters

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
> functions.

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.
(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.

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.


Takashi