[alsa-devel] asihpi driver -> kernel

[Takashi Iwai]

Hi Eliot,

[it seems that you Cc'ed to the old alsa-devel ML so the last one
 seemed missing.]

At Tue, 16 Oct 2007 14:47:15 +1300,
Eliot Blennerhassett wrote:
> 
> Hi Takashi,
> 
> still trying to move forward on this...
> 
> Looking for "what is the first step" change to make in your opinion?

The first step would be to get closer to linux kernel coding style.
Namely,
- get rid of C++ style function names, variables and field names
  (no hungarian!)
- remove API wrappers

Eventually, we may get rid of unneeded typedefs, too.

Then, we can begin with simplifying the driver structure.  Currently,
asihpi and hpi are almost indepedent implementations.  This makes life
harder.


> Takashi Iwai wrote:
> > At Wed, 05 Sep 2007 13:00:35 +1200,
> > Eliot Blennerhassett wrote:
> >> Hi Takashi,
> >>
> >> I'd like to talk more about what you think is needed to get asihpi driver 
> >> ready for kernel tree.
> >>
> >> I think for now we should put aside cosmetic coding style changes (eg. comment 
> >> style, whitespace etc.) these are fixable with combination of automated tools 
> >> and some monkey work.
> >>
> >> So, lets start with the hard part. If we can't solve that there is no point in 
> >> cosmetics!
> >>
> >> A while ago you said "The specific problem I'm concerned is the "glue-code" 
> >> style in HPI driver.  The OS-independent wrapper code is often disliked by 
> >> kernel maintainers. "
> >>
> >> Can you point to specific examples.  Even better suggest how you think it 
> >> should be done better.  Either way I need to consider how your concerns could 
> >> be addressed without breaking our codebase for other OSes etc. 
> >>
> >> thanks and regards
> > 
> > The problem is that the whole "wrapping" is superfluous when you
> > concentrate on Linux driver.  Merging into the linux kernel tree means
> > that the driver would be dedicated to Linux.  It's no longer your
> > child, and it's no longer for every other OS.  Thus, "common codebase"
> > doesn't exist per se.
> > 
> > For example, the whole definitions in hpios_linux_kernel.h should go
> > away.  The driver should use the native kernel API instead.
> 
> Even this is not so hard, as it a mechanical process to replace wrapped
> calls with linux native calls.

OK, then it's fine.

> > I know it's a very hard decision.  Supporting other OSes is very
> > important in most cases.  OTOH, the slim-down is the best way to make
> > the driver code maintainable for other people.  Honestly, I find it
> > also hard to maintain/debug the current ASIHPI code.  It's too
> > different from the normal codes, and understanding it alone takes too
> > much energy before analyzing the problem.
> 
> This is where I need more input, because to _me_ (of course) the code is
> understandable.
> 
> Is it the whole source or just parts that are hard.
> E.g toplevel asihpi.c Ok/No?

The code in asihpi.c is readable enough.  But, other parts are
different.  The difficulty is that the driver structure isn't
straight (apart from the coding style issues).  We have an entry point
for PCI driver _and_ indepdendent HPI initialization.  This is already
puzzling because HPI is also assigned to the corresponding PCI
device.

Also, the message union/struct is another stuff that is pretty hard to
follow.  Since we use no C++, union is often bad for readability.
Could it be a bit simpler?


> > The coding-style issues are similar problems.  When C++-style codes
> > appear in the tree of C-codes, they are strange.  Simply so.  It makes
> > difficult to find the problematic part.  "A buggy part looks buggy" --
> > it's the whole point of having a uniform coding style.
> 
> Again, a mechanical process to change comments and style etc.

Then it's fine with me.  Let's try and see the progress.


> > These problems (different from others) make it impossible to merge to
> > the upstream tree, so far.
> 
> 
> > 
> > 
> >> Eliot
> >> BTW I'm happy to conduct this discussion publically on alsa-devel, some other 
> >> people might learn something too...
> > 
> > I think it's a good thing, too.  Feel free to add alsa-devel to Cc in
> > the next reply with citation.
> > 
> >> For reference here is the source tree diagram.
> >>
> >> asihpi.c alsa driver toplevel
> >>  |
> >> hpifunc.c   message/response pack/unpack
> >>  |
> >> hpimsgx.c  hpi stream ownership, backend selection, some init
> >> |
> >> hpi6205.c hpi6000.c hpi4000.c card family drivers
> >> |
> >> hpicmn.c, hpios_linux.c hpidspcd.c   hpidebug.c  library code
> >>
> >> hpimod.c  probing, HPI ioctl
> >>
> > 
> > 
> > thanks,
> > 
> > Takashi


thanks,

Takashi