[alsa-devel] [PATCH 0/3] ALSA: Add DSP firmware loader

[Takashi Iwai]

At Thu, 30 Aug 2012 13:21:03 -0700,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett at creativelabs.com>
> 
> Hi Takashi,
> Thanks for the recent code samples, they were very useful. We've built on them
> to produce this latest patchset, incorporating the updates you provided to add the loader functions, and doing a minimal update to the CA0132 codec to use the new ops
> to perform the firmware transfer.
> Please let us know if any tweaks are needed to the update.

Thanks for the patches, and sorry for the late response, as I was
traveling for KS and Plumbers.

Looking at the new codes now...  well, the first two patches are
almost what I wrote.  In such a case, you must not change the
authorship.  Keep the git commit author as me.

The third one, the most essential part in the whole patch series, is
still fairly difficult to read.  Basically a few things are missing:

- No comment to functions.  Most of functions look obvious, but still
  better to describe the fundamental mechanism of the DSP loading on
  CA0132.  And give descriptions to arguments that aren't pretty
  intuitive (e.g. how router_chans, reloc or ovly influence on
  behavior.)

- No comment is given to registers.  Hard to guess what's XRAM, YRAM,
  UC or whatever.  Brief and concise descriptions are appreciated.

- Home-baked debug macros like SUCCEEDED(), CTASSERT(), CA0132_LOG()
  or FAIL_MSG() should be avoided.  You might think a hack like
  FAIL_MSG() is great, but it isn't -- even though it saves lines, it
  hinders the code reading unless readers learn this new code flow
  condition.

- Because of the big code shuffling (likely too many lines have been
  inserted in the middle), the patch generation wasn't optimal.  For
  example, you can see chipio_send() is completely added and then
  deleted in the patch although the function itself isn't changed at
  all.  You can avoided this by carefully putting the new codes in the
  file.  Or some git option might give a better result...

- Why hda_stream_format is defined?

- You need proper ifdefs in patch_ca0132.c for the case without
  CONFIG_SND_HDA_DSP_LOADER.

- The firmware data must be cached and reloaded upon PM resume.

Also, you fixed some space issues in the patch.  If a clean-up is
really needed, do it in a separate patch, and don't mix it in this
patch.


thanks,

Takashi