At Thu, 30 Aug 2012 13:21:03 -0700, Ian Minett wrote:
From: Ian Minett ian_minett@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